1540820 - |mach build| rebuilds native code when changing java

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect, P2)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: build.txt

STR

• build geckoview from m-c: ./mach build
• change a trivial java file, e.g. I added a dummy method:

diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
index 53f3510ae6ec..338fa8e7fc1b 100644
--- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
@@ -57,4 +57,6 @@ public final class RuntimeTelemetry {
 
         return result;
     }
+
+    public void testChange() {}
 }

• rebuild
• observe some native stuff is being rebuilt (on my machine it takes about 5min vs 30sec when rebuilding without changes)

See also attached build log.

Comment 1

[Nick Alexander :nalexander [he/him]]

My guess from the build log is that we're accidentally touching the generated JNI wrapper files, causing many things to rebuild. N.b., it's not enough to just witness the JNI wrapper task being invoked: we try not to write files if they haven't changed so the task can be invoked and not spark downstream rebuilds. But that's still what I think is happening, and where I would start investigating.

Comment 2

[Chris Peterson [:cpeterson]]

Agi says mach package doesn't trigger a native code rebuild, so we have a workaround.

Or should this bug move to the "Firefox for Android::Build Config & IDE Support" Bugzilla component?

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Peterson [:cpeterson] from comment #2)

Agi says mach package doesn't trigger a native code rebuild, so we have a workaround.

It's kind of an accident that mach package triggers Java code rebuild at all, but... shrug.

Or should this bug move to the "Firefox for Android::Build Config & IDE Support" Bugzilla component?

Yeah. I just dug into this a little; it's definitely writing JNI wrapper files when it shouldn't. I'm going to go find out why :)

Comment 4

[Nick Alexander :nalexander [he/him]]

This was fallout from Bug 1509572, which moved the "invalidation smarts" to Gradle. Unfortunately, it's not smart enough: there are many situations where the annotations might change (a new method) but where they don't actually change (a new method that isn't annotated with @JNITarget).

Since we don't want to spend the time to make the "invalidation smarts" truly smart, we need to bring back a little bit of Bug 1509572. Patch inbound.

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1540820 - Don't write generated JNI wrappers for every Java-level change. r?agi

This was fallout from Bug 1509572, which moved the "invalidation
smarts" to Gradle. Unfortunately, those smarts are not smart enough:
there are many situations where the annotations might change (a new
method) but where they don't actually change (a new method that isn't
annotated with @JNITarget).

Since we don't want to spend the time to make the "invalidation
smarts" truly smart, we need to bring back this little bit of Bug
1509572.

While we're here, we ensure that there is only one JNI wrapper
generation task for GeckoView and Fennec, regardless of variant.
Right now, those are named like:

• geckoview:generateJNIWrappersForGeneratedWithGeckoBinariesDebug
• app:generateJNIWrappersForFennecWithoutGeckoBinariesDebug

See https://bugzilla.mozilla.org/show_bug.cgi?id=1509539#c1 for some
discussion of these JNI wrapper generation tasks.

Comment 6

[Nick Alexander :nalexander [he/him]]

agi: some local testing would be appreciated. Works fine for me, but could you exercise things for a day as you review? Thanks!

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

From what I can see it works :) Thanks nalexander! I'll keep using it for a day and then approve the code review.

Comment 8

[Marcia Knous [:marcia]]

Marking as P2 per triage.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/744aec2e16db
Don't write generated JNI wrappers for every Java-level change. r=agi

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/744aec2e16db