Closed Bug 1444546 Opened 2 years ago Closed Last year

Migrate build/annotationProcessors to Gradle

Categories

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

enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(6 files)

build/annotationProcessors is the last consumer of config/java-build.mk and add_java_jar, which is close to the last vestige of the moz.build-based Java build system.

This ticket tracks migrating that JAR to a new Gradle project, and figuring out how to express the current consumers of the JAR (SDK bindings and JNI wrappers) as GENERATED_FILES that use the new Gradle project.  Lots of interconnected pieces!

This will likely subsume Bug 1361466 and Bug 1064554, although it's not identical to either.  It's not Bug 1067217, although it will make that a lot easier to tackle.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da246bbcf3a253f1d44a3ad08af470a8750806e is looking pretty healthy.  I have no intuition for how well this will work in practice:

- changes to the export tier aren't picked up by |mach build binaries|
- regular |mach build| will trigger lots of Gradle "builds", since there's no way to know that the native integration points haven't changed without actually invoking Gradle.  Those "builds" should be pretty quick, but it's still a few seconds on every regular build.

I'm hoping the GeckoView engineers can play with this and we can work to iron out kinks.
Comment on attachment 8967159 [details]
Bug 1444546 - Part 1: hg mv build/annotationProcessors mobile/android/annotations.

https://reviewboard.mozilla.org/r/235820/#review241960
Attachment #8967159 - Flags: review?(nchen) → review+
Comment on attachment 8967160 [details]
Bug 1444546 - Part 2: Build annotationProcessors with Gradle.

https://reviewboard.mozilla.org/r/235822/#review241962
Attachment #8967160 - Flags: review?(nchen) → review+
Comment on attachment 8967161 [details]
Bug 1444546 - Part 3: Use GENERATED_FILES for Android SDK bindings generation.

https://reviewboard.mozilla.org/r/235824/#review241964

::: mobile/android/annotations/src/main/java/org/mozilla/gecko/annotationProcessors/SDKProcessor.java:256
(Diff revision 1)
> -                "#ifndef " + generatedFilePrefix + "_h__\n" +
> +                    "#ifndef " + generatedFilePrefix + "_h__\n" +
> -                "#define " + generatedFilePrefix + "_h__\n" +
> +                            "#define " + generatedFilePrefix + "_h__\n" +

We should keep previous alignment.

::: mobile/android/annotations/src/main/java/org/mozilla/gecko/annotationProcessors/SDKProcessor.java:268
(Diff revision 1)
> -                "#include \"" + generatedFilePrefix + ".h\"\n" +
> +                    "#include \"" + generatedFilePrefix + ".h\"\n" +
> -                "#include \"mozilla/jni/Accessors.h\"\n" +
> +                            "#include \"mozilla/jni/Accessors.h\"\n" +

Alignment

::: mobile/android/annotations/src/main/java/org/mozilla/gecko/annotationProcessors/SDKProcessor.java:307
(Diff revision 1)
> -                "} /* sdk */\n" +
> +                    "} /* sdk */\n" +
> -                "} /* java */\n" +
> +                            "} /* java */\n" +

Alignment

::: mobile/android/annotations/src/main/java/org/mozilla/gecko/annotationProcessors/SDKProcessor.java:312
(Diff revision 1)
> -                "} /* sdk */\n" +
> +                    "} /* sdk */\n" +
> -                "} /* java */\n" +
> +                            "} /* java */\n" +

Alignment

::: mobile/android/geckoview/build.gradle:313
(Diff revision 1)
> +    // com.android.tools.lint.bindir pointing to the ANDROID_SDK tools
> +    // directory"
> +    systemProperties = [
> +        'com.android.tools.lint.bindir': "${android.sdkDirectory}/tools",
> +    ]
> +    

Extra spaces

::: mobile/android/geckoview/build.gradle:326
(Diff revision 1)
> +        // From -Pgenerate_sdk_bindings_args=... on command line.
> +        args project.generate_sdk_bindings_args.split(':')
> +    }
> +
> +    workingDir "${topsrcdir}/widget/android/bindings"
> +            

Extra spaces
Attachment #8967161 - Flags: review?(nchen) → review+
Comment on attachment 8967162 [details]
Bug 1444546 - Part 4: Use GENERATED_FILES for Android JNI wrapper generation.

https://reviewboard.mozilla.org/r/235826/#review241968
Attachment #8967162 - Flags: review?(nchen) → review+
Comment on attachment 8967163 [details]
Bug 1444546 - Part 4b: Make AnnotationProcessor avoid writing, like Python's FileAvoidWrite.

https://reviewboard.mozilla.org/r/235828/#review241970

::: mobile/android/annotations/src/main/java/org/mozilla/gecko/annotationProcessors/AnnotationProcessor.java:171
(Diff revision 1)
>          return name.replaceAll("\\W", "_");
>      }
>  
> -    private static void writeOutputFile(final String name,
> -                                        final StringBuilder content) {
> -        FileOutputStream outStream = null;
> +    private static int writeOutputFile(final String name, final StringBuilder content) {
> +
> +        final byte[] contentBytes = content.toString().getBytes();

I think we should use `.getBytes(StandardCharsets.UTF_8)`.
Attachment #8967163 - Flags: review?(nchen) → review+
This is awesome! Thanks!
jchen: snorp: could I get some local development testing with these patches?  I don't build native code enough to see how painful this is in practice.  Just apply the series onto central and go about your business; let me know how it goes.
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
Attachment #8967161 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8967161 [details]
Bug 1444546 - Part 3: Use GENERATED_FILES for Android SDK bindings generation.

https://reviewboard.mozilla.org/r/235824/#review243326

I think this is fine; I am not 100% clear on the consequences of the `GENERATED_FILES` output issue below, but we can probably just land it as-is?

::: build/autoconf/android.m4
(Diff revision 1)
> -AC_MSG_CHECKING([for Android lint classpath])
> -ANDROID_LINT_CLASSPATH=""

Removing this is fantastic.

::: widget/android/bindings/moz.build:35
(Diff revision 1)
>  # and the pattern rule is matched but doesn't resolve both sources, causing a
>  # failure.  Adding the SOURCES to GENERATED_FILES causes the sources
>  # to be built at export time, which is before MediaCodec.o needs them; and by
>  # the time MediaCodec.o is built, the source is in place and the VPATH
>  # resolution works as expected.
> -GENERATED_FILES += [f[1:] for f in SOURCES]
> +GENERATED_FILES += [stem + '.h' for stem in generated]

Nit: this uses string concatenation, whereas the other (visible) list comprehensions over `generated` use `%`.  Just use `%` for consistency?

::: widget/android/bindings/moz.build:37
(Diff revision 1)
> +# The recursive make backend treats the first output specially: it's passed as
> +# an open FileAvoidWrite to the invoked script.  That doesn't work well with
> +# the Gradle task that generates all of the outputs, so we add a dummy first
> +# output.

Does this mean that the Python script should write some dummy data or something to `sdk_bindings`?  I think the `GENERATED_FILES` machinery always updates the timestamp, so I think this Just Works now (but may cause unnecessary rebuilds?).  But in a world where `GENERATED_FILES` is smarter, I think this hackery causes pain.
Attachment #8967161 - Flags: review?(nfroyd) → review+
Attachment #8967162 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8967162 [details]
Bug 1444546 - Part 4: Use GENERATED_FILES for Android JNI wrapper generation.

https://reviewboard.mozilla.org/r/235826/#review243338

r=me with the same caveats on the dummy `GENERATED_FILES` bit noted in the part 3 review.
Attachment #8967162 - Flags: review?(nfroyd) → review+
Attachment #8967164 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8967164 [details]
Bug 1444546 - Post: Remove add_java_jar and support.

https://reviewboard.mozilla.org/r/235830/#review243340

Oh man, so great to delete code in the new build system itself.

::: python/mozbuild/mozbuild/frontend/data.py
(Diff revision 1)
> -class ContextWrapped(ContextDerived):
> -    """Generic context derived container object for a wrapped rich object.

I guess we have this class because we never got around to implementing `with` scopes for moz.build variables?
Attachment #8967164 - Flags: review?(nfroyd) → review+
I've been doing some more work that involved changing JNI extensively, and haven't noticed any issues. I think it's good to go!
Flags: needinfo?(nchen)
I also haven't noticed any issues with this.
Flags: needinfo?(snorp)
Just noticed an issue with SDK headers: if I change the annotation processor code (e.g. CodeGenerator.java) and build, the GeneratedJNI* files are automatically updated, but the SDK headers (e.g. MediaCodec.h) are not updated and require manual clobber.
Comment on attachment 8967161 [details]
Bug 1444546 - Part 3: Use GENERATED_FILES for Android SDK bindings generation.

https://reviewboard.mozilla.org/r/235824/#review243326

> Does this mean that the Python script should write some dummy data or something to `sdk_bindings`?  I think the `GENERATED_FILES` machinery always updates the timestamp, so I think this Just Works now (but may cause unnecessary rebuilds?).  But in a world where `GENERATED_FILES` is smarter, I think this hackery causes pain.

I think the point is that `GENERATED_FILES` _doesn't_ update the timestamp unless the outputs change.  This is working well in practice (here and in other situations), so I don't think it needs to be addressed for this patch.  I could imagine changing things as we make `GENERATED_FILES` more robust (for tup and other backends).
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/591fee0c3b32
Part 1: hg mv build/annotationProcessors mobile/android/annotations. r=jchen
https://hg.mozilla.org/integration/autoland/rev/d8c9882a3b3c
Part 2: Build annotationProcessors with Gradle. r=jchen
https://hg.mozilla.org/integration/autoland/rev/731eb2e4b557
Part 3: Use GENERATED_FILES for Android SDK bindings generation. r=froydnj,jchen
https://hg.mozilla.org/integration/autoland/rev/f0a4cabc8c94
Part 4: Use GENERATED_FILES for Android JNI wrapper generation. r=froydnj,jchen
https://hg.mozilla.org/integration/autoland/rev/4d69ed596b02
Part 4b: Make AnnotationProcessor avoid writing, like Python's FileAvoidWrite. r=jchen
https://hg.mozilla.org/integration/autoland/rev/22f959f6f58c
Post: Remove add_java_jar and support. r=froydnj
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.