Closed Bug 1384312 Opened 7 years ago Closed 7 years ago

Support generating JNI wrappers under --with-gradle

Categories

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

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: nalexander, Assigned: maliu)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1384312 - Support generating JNI wrappers under --with-gradle,
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1384312 - Support generating JNI wrappers under --with-gradle,
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
In order to transition to --with-gradle builds, we need to support generating JNI wrappers.  This is tricky.

The _right_ way to do this is to complete Bug 1067217 by making the existing annotation processing Java program a real Java Annotation Processor, and then to hook it into the Android-Gradle annotation processor support.  Unfortunately, this has the following dependency chain (see https://developer.android.com/studio/releases/gradle-plugin.html):

- Android-Gradle plugin version 2.3 or later
- Gradle 3.3 or later
- Java 1.8
- Android build-tools 25.0.0 or later

Now, the new build-tools require

- glibc 2.14 or later

and the existing build images (centos6-build-upd, android-gradle-build) are pinned to glibc 2.12.

That's a lot of upgrading in order to get annotation processing!

So we need to investigate a way to get the right JAR files and classpaths to the existing annotation processing program.  That's this ticket.
The transition to Gradle is finally going to happen \o/  I'm going to split this into v1 (now) and v2 (future) meta tickets, and move them out of Core :: Build Config, since they're really Firefox for Android-specific.
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
(In reply to Nick Alexander :nalexander from comment #2)
2> The transition to Gradle is finally going to happen \o/  I'm going to split
> this into v1 (now) and v2 (future) meta tickets, and move them out of Core
> :: Build Config, since they're really Firefox for Android-specific.

Gah!  Wrong ticket.
We're going to do this correctly -- using the `annotationProcessor` capability built into Android-Gradle plugin 2.3+ -- for our initial landing.  This is mostly 'cuz we need to do almost all of the upgrade work to target Android O, but also because figuring out the location of the relevant .class and .jar files for the existing annotation processing program (see #c0) turned out to be challenging.
Depends on: 1366644
No longer blocks: gradle-automation
No longer blocks: gradle-automation-v2
Comment on attachment 8917602 [details]
Bug 1384312 - Support generating JNI wrappers under --with-gradle,

https://reviewboard.mozilla.org/r/188558/#review194104

This is looking pretty good!  Just one substantial comment about finding the JAR tasks.

::: mobile/android/base/Makefile.in:188
(Diff revision 1)
>  FENNEC_JARS += ../stumbler/stumbler.jar
>  endif
>  
> +else # MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE
> +
> +$(gradle_dir)/geckoview/intermediates/bundles/debug/classes.jar: .gradle.deps

Push these two lines below the `*_JARS` declarations, and then make these lines:
```
$(GECKOVIEW_JARS): .gradle.deps
$(FENNEC_JARS): .gradle.deps
```
That'll save us repeating ourselves once.

::: mobile/android/base/Makefile.in:260
(Diff revision 1)
>  	cp $(gradle_dir)/app/intermediates/transforms/dex/officialPhoton/debug/folders/1000/1f/main/classes.dex $@
> +
> +GeneratedJNIWrappers.cpp: .gradle.deps
> +	$(REPORT_BUILD)
> +
> +# This annotation processing step also generates

Rather than commenting, let's be explicit:
```
FennecJNIWrappers.cpp FennecJNIWrappers.h FennecJNINatives.h: .gradle.deps
...
```
(Here and above.)

::: mobile/android/base/Makefile.in:645
(Diff revision 1)
>  FENNEC_AIDLS = \
>    $(NULL)
>  
>  fennec_aidl_src_path := $(srcdir)/aidl
>  fennec_aidl_target_path := generated
> -fennec_aidl_targets := $(addprefix $(fennec_aidl_target_path)/,$(patsubst %.aidl,%.java,$(FENNEC_AIDLS:.java=)))
> +fennec_aidl_targets := $(addprefix $(fennec_aidql_target_path)/,$(patsubst %.aidl,%.java,$(FENNEC_AIDLS:.java=)))

This looks like a little typo -- probably me :)

::: mobile/android/gradle/with_gecko_binaries.gradle:129
(Diff revision 1)
>      android.sourceSets."${sourceSet}".assets.srcDir syncOmnijarFromDistDir.destinationDir
>      android.sourceSets."${sourceSet}".assets.srcDir syncAssetsFromDistDir.destinationDir
>      android.sourceSets."${sourceSet}".jniLibs.srcDir syncLibsFromDistDir.destinationDir
>  }
> +
> +ext.configureJNIWithGeckoBinaries = { variant, module ->

This name doesn't make much sense, since there's nothing about the Gecko binaries involved.  How about `configureVariantWithJNIWrappers`?

::: mobile/android/gradle/with_gecko_binaries.gradle:132
(Diff revision 1)
>  }
> +
> +ext.configureJNIWithGeckoBinaries = { variant, module ->
> +
> +    def jarTask = tasks.findByName("jarOfficialPhotonDebugClasses")
> +    if(jarTask == null){

nit: space between `if` and `(`.  (Here and below.)  Also, this isn't doing the right thing for the _variant_ -- it's always using "OfficialPhotonDebug" or whatever.  Can you figure out how to get the JAR task for the given variant?  You'll need to do something different for application and library variants.

::: mobile/android/gradle/with_gecko_binaries.gradle:144
(Diff revision 1)
> +        throw new GradleException("Jar task output multiple files other than one single jar")
> +    }
> +
> +    task("generateJNIWrappersFor${module}${variant.name.capitalize()}", type: JavaExec) {
> +        classpath = variant.javaCompile.classpath
> +        // throw new GradleException(variant.javaCompile.options.bootClasspath)

nit: kill this.

::: mobile/android/gradle/with_gecko_binaries.gradle:156
(Diff revision 1)
> +
> +        workingDir "${topobjdir}/mobile/android/base"
> +
> +        dependsOn jarTask
> +    }
> +

nit: kill empty line.
Attachment #8917602 - Flags: review?(nalexander) → review-
Comment on attachment 8917602 [details]
Bug 1384312 - Support generating JNI wrappers under --with-gradle,

https://reviewboard.mozilla.org/r/188558/#review194222

::: mobile/android/gradle/with_gecko_binaries.gradle:131
(Diff revisions 1 - 2)
>      android.sourceSets."${sourceSet}".jniLibs.srcDir syncLibsFromDistDir.destinationDir
>  }
>  
> -ext.configureJNIWithGeckoBinaries = { variant, module ->
> +ext.configureVariantWithJNIWrappers = { variant, module ->
>  
>      def jarTask = tasks.findByName("jarOfficialPhotonDebugClasses")

This still doesn't address the fact that the variant might be `local` or something else (so not `official`) and the configuration might not be `debug`.
Attachment #8917602 - Flags: review?(nalexander) → review-
Comment on attachment 8914572 [details]
Bug 1384312 - Part 1: Move omg.annotation; add stub Gradle annotationProcessor.

We're taking a different approach, so this doesn't need review.
Attachment #8914572 - Flags: review?(max)
Attachment #8914572 - Attachment is obsolete: true
Comment on attachment 8917602 [details]
Bug 1384312 - Support generating JNI wrappers under --with-gradle,

https://reviewboard.mozilla.org/r/188558/#review194622

Looks good!
Add something like:
```
-    task("generateJNIWrappersFor${module}${variant.name.capitalize()}", type: JavaExec) {
+    def wrapperTask = task("generateJNIWrappersFor${module}${variant.name.capitalize()}", type: JavaExec) {
         classpath = variant.javaCompile.classpath
         // Include android.jar.
         classpath variant.javaCompile.options.bootClasspath
         classpath "${topobjdir}/build/annotationProcessors/annotationProcessors.jar"
...
         workingDir "${topobjdir}/mobile/android/base"
 
         dependsOn jarTask
     }
+
+ variant.assemble.dependsOn wrapperTask
```
so this happens every build, and let's land it!
Attachment #8917602 - Flags: review?(nalexander) → review+
Pushed by max@mxli.us:
https://hg.mozilla.org/integration/autoland/rev/9a3c4f7cd85e
Support generating JNI wrappers under --with-gradle, r=nalexander
Comment on attachment 8919076 [details]
Bug 1384312 - Support generating JNI wrappers under --with-gradle,

https://reviewboard.mozilla.org/r/189984/#review195176

Max and I worked through these issues together, and jointly reviewed.  Try looks green -- bombs away!
Attachment #8919076 - Flags: review?(nalexander) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fab13cda2355
Support generating JNI wrappers under --with-gradle, r=nalexander
https://hg.mozilla.org/mozilla-central/rev/fab13cda2355
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee: nobody → max
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 58 → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: