1418464 - Do less work XZ compressing when packaging GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

It's nice that we have an Android Studio project that allow us to build geckoview_example and geckoview, but it would be nice if hitting the 'run' button would also bring in any changes to gecko itself. The workflow is basically:

1) Add a new method to GeckoView
2) Implement above method in Gecko using JS or native code
3) Write a test or new API in GeckoViewExample or other sibling project
4) Win

Since setting up a gecko build is such an involved process I think we could just punt on that. We'd probably just assume that 'mach build && mach package' works. I don't think we really want Studio to install a mozconfig, for instance. The instructions for getting this to work for developers would basically be 'mach bootstrap', set up mozconfig, then ensure 'mach configure' works

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

It's very easy to run 'mach build binaries' as a dependency of the current 'syncLibsFromDistDir' task, however, 'mach build binaries' does not put stuff in the right place. That happens as part of the 'mach package' step currently, so this will be a little annoying.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1418464 - Trigger build of Gecko sources from gradle for non-official audience

Review commit: https://reviewboard.mozilla.org/r/204378/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204378/

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933459 [details]
Bug 1418464 - Trigger build of Gecko sources from gradle for non-official audience

Nick this is probably not quite right but does seem to improve things. The main changes are:

1) Add a new 'geckoSources' module that is similar to the 'omnijar' one. It has most of the Gecko sources as inputs.
2) Add a packageOmnijarAndLibs task which simply runs 'mach package'. This replaces buildOmnijars. It uses 'dist/*.so' and the omnijar module as inputs, so it should only run when those things change.
3) Add a buildGeckoBinaries task, which the above depends on. It uses the 'geckoSources' inputs and runs 'mach build binaries' when changes are detected.

The downside is that only changing JS files will be slower now. We could run 'mach package' if there are binary changes and only run the gradle-omnijar Makefile target otherwise, but I think this is a lot easier to understand.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8933459 [details]
Bug 1418464 - Trigger build of Gecko sources from gradle for non-official audience

https://reviewboard.mozilla.org/r/204378/#review209932

r- is f+.  I have some questions, many nits, and one real concern about |mach package| being much, much slower than the special handling we were doing before.  We could make the task that produces the omnijar depend on `COMPILE_ENVIRONMENT` and address that concern easily.  That is, make the task itself choose the logic about |mach package| vs |gradle targets|.  Does that make sense?

::: mobile/android/app/geckoSources/build.gradle:1
(Diff revision 1)
> +buildDir "${topobjdir}/gradle/build/mobile/android/geckoSources"

We set a pattern for `snake_case` with `geckoview_example`.  Is there a reason to deviate?

In addition, I think Gecko sources probably doesn't mean much to new folks -- `native_code`, perhaps?

::: mobile/android/app/geckoSources/build.gradle:7
(Diff revision 1)
> +
> +apply plugin: 'java'
> +
> +sourceSets {
> +    main {
> +        // Depend on the Gecko resources in mobile/android.

nit: the comment is wrong, and we should be clear on why we're choosing this set.  Also, sort alphabetically.

::: mobile/android/gradle/with_gecko_binaries.gradle:15
(Diff revision 1)
> +evaluationDependsOn(':geckoSources')
>  
> -task buildOmnijars(type:Exec) {
> +task buildGeckoBinaries(type:Exec) {
>      dependsOn rootProject.generateCodeAndResources
>  
>      // See comment in :omnijar project regarding interface mismatches here.

nit: update comment.

::: mobile/android/gradle/with_gecko_binaries.gradle:16
(Diff revision 1)
>  
> -task buildOmnijars(type:Exec) {
> +task buildGeckoBinaries(type:Exec) {
>      dependsOn rootProject.generateCodeAndResources
>  
>      // See comment in :omnijar project regarding interface mismatches here.
> -    inputs.file(project(':omnijar').sourceSets.main.resources.srcDirs).skipWhenEmpty() 
> +    inputs.file(project(':geckoSources').sourceSets.main.resources.srcDirs).skipWhenEmpty() 

nit: cull trailing whitespace throughout, perhaps as a Pre.

::: mobile/android/gradle/with_gecko_binaries.gradle:18
(Diff revision 1)
>      dependsOn rootProject.generateCodeAndResources
>  
>      // See comment in :omnijar project regarding interface mismatches here.
> -    inputs.file(project(':omnijar').sourceSets.main.resources.srcDirs).skipWhenEmpty() 
> +    inputs.file(project(':geckoSources').sourceSets.main.resources.srcDirs).skipWhenEmpty() 
>  
>      // Produce both the Fennec and the GeckoView omnijars.

nit: update comments!

::: mobile/android/gradle/with_gecko_binaries.gradle:31
(Diff revision 1)
> +}
> +
> +task packageOmnijarAndLibs(type:Exec) {
> +    dependsOn rootProject.generateCodeAndResources, buildGeckoBinaries
> +
> +    inputs.files(fileTree(dir: "${topobjdir}/dist/bin", include: '*.so')).skipWhenEmpty()

I'm not 100% that multiple `skipWhenEmpty` definitions does what you think it does.  Are you?

::: mobile/android/gradle/with_gecko_binaries.gradle:38
(Diff revision 1)
>      outputs.file "${topobjdir}/dist/fennec/assets/omni.ja"
>      outputs.file "${topobjdir}/dist/geckoview/assets/omni.ja"
>  
> -    workingDir "${topobjdir}"
> +    workingDir "${topsrcdir}"
>  
> -    commandLine mozconfig.substs.GMAKE
> +    commandLine './mach'

This hurts.  I'd really prefer not to do this in the general case.  We can discuss the mechanics further.

::: mobile/android/gradle/with_gecko_binaries.gradle
(Diff revision 1)
> -    commandLine mozconfig.substs.GMAKE
> -    args '-C'
> +    commandLine './mach'
> +    args 'package'
> -    args "${topobjdir}/mobile/android/base"
> -    args 'gradle-omnijar'
> -
> -    // Only show the output if something went wrong.

I think you want these debugging stanzas for both no matter what.  Why would you drop them?

::: mobile/android/gradle/with_gecko_binaries.gradle:42
(Diff revision 1)
> -            throw new GradleException("Process '${commandLine}' finished with non-zero exit value ${execResult.exitValue}:\n\n${standardOutput.toString()}")
> -        }
> -    }
>  }
>  
> +

nit: drop extraneous newlines.

::: mobile/android/gradle/with_gecko_binaries.gradle:96
(Diff revision 1)
>      // moz.build system, which can be re-entrant.  Official builds are driven by
>      // the moz.build system and should never be re-entrant in this way.
>      if (!((variant.productFlavors*.name).contains('official'))) {
> -        syncOmnijarFromDistDir.dependsOn buildOmnijars
> +        syncOmnijarFromDistDir.dependsOn packageOmnijarAndLibs
> +
> +        // We want to pick up changes from cpp files

You want `official` _and_ `withGeckoBinaries` for this, don't you?

::: settings.gradle:31
(Diff revision 1)
>  // local.properties (first 'sdk.dir', then 'android.dir') and then the
>  // environment variable ANDROID_HOME will override this.  That's unfortunate,
>  // but it's hard to automatically arrange better.
>  System.setProperty('android.home', json.substs.ANDROID_SDK_ROOT)
>  
> -include ':app', ':geckoview_test'
> +include ':app'

Looks like this change bled across tickets.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933459 [details]
Bug 1418464 - Trigger build of Gecko sources from gradle for non-official audience

https://reviewboard.mozilla.org/r/204378/#review209932

> We set a pattern for `snake_case` with `geckoview_example`.  Is there a reason to deviate?
> 
> In addition, I think Gecko sources probably doesn't mean much to new folks -- `native_code`, perhaps?

How about just 'natives'? We use that terminology on in the JNI bindings so I think it kinda makes sense.

> I'm not 100% that multiple `skipWhenEmpty` definitions does what you think it does.  Are you?

My reading of this doc suggests that it does the obvious thing: https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/TaskInputFilePropertyBuilder.html#skipWhenEmpty()

> I think you want these debugging stanzas for both no matter what.  Why would you drop them?

The build and package steps can take a long time if there is work to do. I found the experience to be better if you see the normal output scroll by, and gradle seems to already do the right thing when the process exits with error.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933459 [details]
Bug 1418464 - Trigger build of Gecko sources from gradle for non-official audience

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204378/diff/1-2/

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8933459 [details]
Bug 1418464 - Trigger build of Gecko sources from gradle for non-official audience

https://reviewboard.mozilla.org/r/204378/#review211920

r- just to check the guarding.

Please put some of the details of how this works into the commit message, as well.

::: mobile/android/gradle/with_gecko_binaries.gradle:55
(Diff revision 2)
> +    args 'build'
> +    args 'binaries'
> +}
> +
> +task packageOmnijarAndLibs(type:Exec) {
> +    dependsOn rootProject.generateCodeAndResources, buildGeckoBinaries

tiny nit: two lines, please, for ease of reading.

::: mobile/android/gradle/with_gecko_binaries.gradle:57
(Diff revision 2)
> +}
> +
> +task packageOmnijarAndLibs(type:Exec) {
> +    dependsOn rootProject.generateCodeAndResources, buildGeckoBinaries
> +
> +    inputs.files(fileTree(dir: "${topobjdir}/dist/bin", include: '*.so')).skipWhenEmpty();

You are confident two `skipWhenEmpty` invocations does the right thing?  (I asked before and saw no reply.)

::: mobile/android/gradle/with_gecko_binaries.gradle:122
(Diff revision 2)
>      if (!((variant.productFlavors*.name).contains('official'))) {
> +        // We want to pick up changes from cpp files when building a
> +        // withGeckoBinaries variant. We also run a full 'mach package' step
> +        // in this case since we need that to do the appropriate stripping and
> +        // compressing of libxul.so, etc.
> +        if ((variant.productFlavors*.name).contains('withGeckoBinaries') &&

nit: one line, brace on same line.  You C++ coders!

::: settings.gradle:36
(Diff revision 2)
>  include ':app'
>  include ':geckoview'
>  include ':geckoview_example'
>  include ':geckoview_test'
>  include ':omnijar'
> +include ':natives'

This all needs to be guarded if `!COMPILE_ENVIRONMENT`, but otherwise it looks pretty good.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> Comment on attachment 8933459 [details]
> Bug 1418464 - Trigger build of Gecko sources from gradle for non-official
> audience
> 
> https://reviewboard.mozilla.org/r/204378/#review209932
> 
> > We set a pattern for `snake_case` with `geckoview_example`.  Is there a reason to deviate?
> > 
> > In addition, I think Gecko sources probably doesn't mean much to new folks -- `native_code`, perhaps?
> 
> How about just 'natives'? We use that terminology on in the JNI bindings so
> I think it kinda makes sense.

I like it.

> > I'm not 100% that multiple `skipWhenEmpty` definitions does what you think it does.  Are you?
> 
> My reading of this doc suggests that it does the obvious thing:
> https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/
> TaskInputFilePropertyBuilder.html#skipWhenEmpty()

Sorry, I missed this!  Roll on.

> > I think you want these debugging stanzas for both no matter what.  Why would you drop them?
> 
> The build and package steps can take a long time if there is work to do. I
> found the experience to be better if you see the normal output scroll by,
> and gradle seems to already do the right thing when the process exits with
> error.

OK.  I'm fine with whatever the actual users want.

Sorry for missing your responses the first time through -- my error.

Comment 9

[Nick Alexander :nalexander [he/him]]

I picked this up for a few hours over the last few days, building on top of WIP patches that generate the JNI wrappers from the Java sources dynamically.

My conclusion: with that functionality, it's much more complicated than this patch series can accommodate.  We need to:

- handle the fact that the different Gradle configurations can, in theory, produce different sets of JNI wrappers
- invoke |mach build binaries| at most once
- try to not run |mach build binaries| unnecessarily

I haven't gotten all of the details straight yet.  My WIP patches write the .h and .cpp too aggressively; we need to FileAvoidWrite them to stop rebuilding as aggressively.

This is trickier than expected because there's a divergence between |mach build| and developer build Gradle variants; really, there's a plurality of Gradle variants that require work to integrate against the single |mach build| target.  It's tricky.

Comment 10

[Nick Alexander :nalexander [he/him]]

It's worth noting that Bug 1444546 solidified a lot of the prerequisite work for this ticket, although nothing has fundamentally improved.

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1418464 - Part 1: XZ compress in the packager rather than package_fennec_apk.py.

By doing this in the packager, it makes it easier to incorporate the
strip and XZ compress logic into the local Gradle build process.

To that end, this patch makes XZ compression a little more explicit in
package-manifest.in and lifts the logic next to the existing logic for
stripping.  Since we only want to XZ compress assets/ (and not libs/),
we need a new flag.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1418464 - Part 2: Do less work when packaging GeckoView.

There are two significant parts to this commit.  The first avoids
scanning for duplicates in the omnijar when packaging locally.  The
Fennec/GeckoView local development edit-test-compile loop _always_
includes packaging, so these developers always pay to scan for
duplicates.  And, for historical reasons (Bug 1351000), we build both
a Fennec and a GeckoView omnijar, so these developers pay to scan
twice!  Since scanning for duplicates isn't something that local
developers are likely to act on, let's not do this at all (rather
than, say, only once for Fennec).

The second avoids stripping and XZ compressing Fennec/GeckoView asset/
libraries twice.  A little path hacking allows to exploit the fact
that the executable processing is idempotent, saving a significant
amount of time during |mach package|.

The final part of this commit just reduces the verbosity of a
`zipalign` invocation.

Depends on D7314

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc793c2d8cee
Part 1: XZ compress in the packager rather than package_fennec_apk.py. r=mshal
https://hg.mozilla.org/integration/autoland/rev/d6794cb231e1
Part 2: Do less work when packaging GeckoView. r=mshal,jchen

Comment 14

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1418464) for test_packager.py build bustages

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=203718442&revision=d6794cb231e188d3bba25f8bae557de19febc09b

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&selectedJob=203718442&searchStr=linux%2Copt%2Cbuild-linux%2Fopt%2C%28b%29

backout: https://hg.mozilla.org/integration/autoland/rev/9a8856c931e52ab1a4c0327011658e44b8342a48

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a32b2b41d1f7
Part 1: XZ compress in the packager rather than package_fennec_apk.py. r=mshal
https://hg.mozilla.org/integration/autoland/rev/bcfb279c5ee8
Part 2: Do less work when packaging GeckoView. r=mshal,jchen

Comment 16

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a32b2b41d1f7
https://hg.mozilla.org/mozilla-central/rev/bcfb279c5ee8

Comment 17

[Nick Alexander :nalexander [he/him]]

Relanded (green) so clearing NI.

Comment 18

[Chris Peterson [:cpeterson]]

63=wontfix because we don't need to uplift this Gradle fix to GV Beta.