1108782 - Pin to explicit versions of static AAR files to supply mobile/android dependencies

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

Right now, we use mozconfig switches, environment variables, and configure.in/autoconf inspection to find pieces of the Android build toolchain and development environment.  We're very forgiving about what we allow: we tend to allow all permutations of the build chain, only infrequently requiring particular versions of particular tools.

This results in the following major con and pro:

CON: folks building mobile/android often find their (new or old) toolchain is no longer supported.  Since the Java/Android toolchain is exercised only during the app tier, they only discover this near the end of their build.

PRO: in general, we support the heterogeneous build environment piece-meal, addressing issues one at a time.  That means a developer who wants to bump a toolchain builder can do so without expecting to fix major fallout in areas disjoint from the toolchain feature she requires.

It's my belief that the CON far outweights the PRO.  In response, I'd like to pin some tools to specified known good versions (for mobile/android only -- we share configuration code with gonk/gaia and I don't want them to change a thing).

The two parts of the chain that should be pinned ASAP are the "build-time Android platform" -- like android-21 -- and the build tools themselves -- like build-tools/21.1.1.

Comment 1

[Nick Alexander :nalexander [he/him]]

It's worth noting that this will unify what local developers are building against with what we run on the build bots.  In practice, this is what we all do anyway, but it's good to enforce it at configure time.

It's also worth noting that Gradle has moved to this approach: the build platform and the build tools are both pinned at build time.

I talked with gps about how this should be achieved technically.  I conclude that we add configure.in checks for specific versions that are optionally set in mobile/android/confvars.sh.  In practice this may not work because android.m4 may find versions that are perhaps not the pinned versions, even though the pinned versions exist.  We'll have to experiment.

glandium, do you have an opinion on whether this is a bad idea or how it should be achieved?

Comment 2

[Mike Hommey [:glandium]]

> glandium, do you have an opinion on whether this is a bad idea or how it should be achieved?

I have no opinion, as I have no idea what problem you're trying to fix.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #2)
> > glandium, do you have an opinion on whether this is a bad idea or how it should be achieved?
> 
> I have no opinion, as I have no idea what problem you're trying to fix.

Was my request too narrowly scoped, i.e. you did read the description in comment 0?  Or is what I wrote there not clear and I need to re-phrase?

Comment 4

[Mike Hommey [:glandium]]

Probably the latter.

Comment 5

[Nick Alexander :nalexander [he/him]]

Observe that the SDK and NDK versions are pinned in automation here:

http://hg.mozilla.org/mozilla-central/annotate/tip/mobile/android/config/mozconfigs/common

Comment 6

[Mike Hommey [:glandium]]

Here's a comment from me as a person who sometimes but rarely builds fennec: *every* *single* *time* I have to build Fennec, the build fails because my sdk or ndk is not up-to-date, most often the former.

configure.in currently says sdk 17 is the minimum supported, and mine is 19. And that didn't work. Before I figured this didn't work, I had errors because the sdk build or platform tools were tool old, so the first thing I did was to upgrade that without touching the sdk version. So I actually had to fail two consecutive builds instead of being rejected by configure.

I laud that you're trying to make things build with a broad range of options, but the reality is that the broad range of options is not as broad as it's defined in configure.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #6)
> Here's a comment from me as a person who sometimes but rarely builds fennec:
> *every* *single* *time* I have to build Fennec, the build fails because my
> sdk or ndk is not up-to-date, most often the former.

I think most infrequent builders suffer in this way, and it sucks.  Hard to get the first build up and running, too :(

> configure.in currently says sdk 17 is the minimum supported, and mine is 19.
> And that didn't work. Before I figured this didn't work, I had errors
> because the sdk build or platform tools were tool old, so the first thing I
> did was to upgrade that without touching the sdk version. So I actually had
> to fail two consecutive builds instead of being rejected by configure.

Aye, this is frustrating.

> I laud that you're trying to make things build with a broad range of
> options, but the reality is that the broad range of options is not as broad
> as it's defined in configure.

This ticket is specifically about *narrowing* the set of options we'll accept during configure.  That's against the thing you say is positive.  I interpret this to mean you see goal of multiple toolchains as positive but agree with me that having the options be narrow and always correct is better than having broad options that are sometimes incorrect.  That is, you support this ticket.  Correct me if I'm mistaken.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #7)
> This ticket is specifically about *narrowing* the set of options we'll
> accept during configure.  That's against the thing you say is positive.  I
> interpret this to mean you see goal of multiple toolchains as positive but
> agree with me that having the options be narrow and always correct is better
> than having broad options that are sometimes incorrect.  That is, you
> support this ticket.  Correct me if I'm mistaken.

There is only one thing I disagree in this paragraph: my experience is not that it's sometimes incorrect. It's that it's *always* incorrect.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #5)
> Observe that the SDK and NDK versions are pinned in automation here:
> 
> http://hg.mozilla.org/mozilla-central/annotate/tip/mobile/android/config/
> mozconfigs/common

One would hope so, but building Fennec (right now) observably fails with r8e on x86, which is indicated there. Turns out we're using r10e despite what is in those configs.

Comment 10

[Nick Alexander :nalexander [he/him]]

The following values are from my $OBJDIR/config.status:

    362:    (''' ANDROID_SDK_ROOT ''', r''' /usr/local/opt/android-sdk '''),
    363:    (''' ANDROID_SDK ''', r''' /usr/local/opt/android-sdk/platforms/android-22 '''),

Part of this might be to require a particular version of the SDK.  Right now we derive ANDROID_SDK_ROOT from ANDROID_SDK, see https://dxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4?offset=0#284.  If we made ANDROID_SDK_ROOT the input, we could version the ANDROID_SDK ourselves, keeping people on the supported version.  That's a good thing.

We would need to bump a lot of mozconfigs, since we have a lot of

ac_add_options --with-android-sdk="/usr/local/opt/android-sdk/platforms/android-22"

in the wild.  sebastian, do you agree this is a good idea?

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #10)
> sebastian, do you agree this is a good idea?

I like the idea of pinning the SDK version even though the no-going-back versioning of artifacts is currently the bigger pain (support library, play services) but this is a different story (consuming AARs).

I don't know where ./mach bootstrap gets its numbers from but would it be possible to keep these two in sync (bootstrap downloads the same version that the config uses)?

Thinking of the upcoming switch to SDK 23 it would be nice if we could bump the version for everyone so that they are not facing obscuce build errors after pulling but at the same time are notified about SDK 23 missing if they don't have it.

Comment 12

[Himani]

I have gone through all the documentation on Building fennec , now the core problem lies that i am trying to build Android -22 but my support library is of version 23 , And I could not find a way to downgrade my version to 22 , now after several failed attempts to build the fennec , I am forced to comment in here :(

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

Bug 1108782 - Part 1: straighten out Java classpaths. r?glandium

This commit is us getting out of our own way.  We were specifying
-classpath twice, once in $(JAVAC) and once in java-build.mk.  Only
the latter of these is active.  This a problem for ANDROID_EXTRA_JARS
-- those JARs should be on the classpath and input to $(DX) -- and
JARs that should be on the classpath but *not* input to $(DX).  This
commit removes the global flags to $(JAVAC) and adds
JAVA_{BOOT}CLASSPATH_JARS.  This required some hijinkery moving
wildcards to moz.build files, but everything seems to work.

As well as clarifying some parts of the build, part 2 uses this work
to modify the classpath.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

Bug 1108782 - Part 2: Add ANDROID_LIBRARIES to moz.build. r?glandium

This gets us a limited version of AAR support: we can consumer static
AAR libraries, where here static does not refer to linking, but to
static assets that are fixed at build-backend time and not modified
(or produced) during the build.  This lets us pin our dependencies
(and move to Google's versioned Maven repository packages, away from
Google's unversioned ad-hoc packages).

By restricting to static AAR libraries, we avoid having to handle
truly complicated dependency trees, as changing parts of generated AAR
files require delicate rebuilding of the APKs (and internal libraries)
that depend on the AAR files.

It is possible that we will generate AARs in the tree at some time.
Right now, we don't do that, even for GeckoView: the AARs produced are
assembled as artifacts at package time and are intended for external
consumption.  We might want this for GeckoView and Fennec at some
time; we should consider using Gradle everywhere at that point.

The patch itself does the simplest possible thing (which has precedent
from Gradle and other build systems): it simply "explodes" the AAR
into the object directory and uses existing mechanisms to refer to the
exploded pieces.

There's a lot not to like in this approach:
* exploding the AAR at build-backend time will slow that process;
* the configure.in variables are multiplying;
* we need to manually reference internal AAR libs;
* I haven't separated the pinned version numbers out of configure.in.
However, it's closer to what we want than what we have!

Comment 15

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/17803/#review15903

Heh, I see now that the variable name in the commit message is wrong.  I'll address with other changes.

Comment 16

[Nick Alexander :nalexander [he/him]]

sebastian: it would be super helpful if you could look over this and especially test this locally.

Comment 17

[Nick Alexander :nalexander [he/him]]

Note to self: check APK size difference (after Proguarding) using GPS -all and GPS {-base,-cast}.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8655078 [details]
MozReview Request: Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

https://reviewboard.mozilla.org/r/17805/#review15963

::: config/makefiles/java-build.mk:26
(Diff revision 1)
> +# Arg 2: Possibly empty list of JAR files.

I'm not sure that $(call callpath_template,-classpath,$(var)) is better than $(addprefix -classpath,$(call classpath_template,$(var))). It's shorter, for sure, but I'm not sure which one makes it clearer what the flag is. I think I tend towards the latter.

::: config/makefiles/java-build.mk:52
(Diff revision 1)
> +		$(call classpath_template,-classpath,$(default_classpath_jars) $(ANDROID_CLASSPATH_JARS) $(ANDROID_EXTRA_JARS))

Why aren't $(default_classpath_jars) and $(default_bootclasspath_jars) in the dependencies?

::: mobile/android/tests/background/junit3/Makefile.in
(Diff revision 1)
> -GARBAGE += AndroidManifest.xml

Removing this line is unrelated.

::: mobile/android/tests/background/junit3/moz.build:25
(Diff revision 1)
> +    TOPOBJDIR + '/mobile/android/base/sync-thirdparty.jar',

Is there an open bug about making those Path instances?

::: mobile/android/tests/background/junit3/moz.build:31
(Diff revision 1)
> +    ]

I guess this is what you were asking for for a globally available variable. What we usually do for things like this is to use includes.

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

https://reviewboard.mozilla.org/r/17807/#review15965

::: build/autoconf/android.m4:421
(Diff revision 1)
> +    AC_SUBST(ANDROID_RECYCLERVIEW_V7_AAR_LIB)

Urgh repetitive code with long variable names, long paths.

Tell me if I'm wrong, but it looks like you need the exact same things for each aar: for $foo-$ver.aar, you need:
 - $foo/res
 - $foo/$foo-$ver-classes.jar
and in some cases
 - $foo/libs/$foo-internal_impl-$ver.jar

It also seems the aar files are all $ANDROID_SDK_ROOT/extras/android/m2repository/$base/$foo/$ver/$foo-$ver.aar.

So all in all, it seems to me there could be a simple AC_DEFUN that does the minimal checks, given $base, $foo and $ver, possibly with a verbose name for the error message (like "Android Support Repository").

This would make:
- the review,
- updating a version,
- adding a new aar
easier.

I know m4/autoconf macros are not the easiest thing, so here are some bases to get you started:

AC_DEFUN([concat],[$1$2$3$4])
AC_DEFUN([MOZ_ANDROID_AAR],[
  define([local_aar_var_base], translit($1, [-a-z], [_A-Z]))
  concat(MOZ_ANDROID_, local_aar_var_base, _AAR_LIB)="$MOZ_BUILD_ROOT/dist/exploded-aar/$1-$2/$1-$2-classes.jar"
  AC_SUBST(concat(MOZ_ANDROID_, local_aar_var_base, _AAR_LIB))
])

::: python/mozbuild/mozbuild/backend/recursivemake.py:1410
(Diff revision 1)
> +        os.rename(mozpath.join(destdir, 'classes.jar'), classes_jar)

And here I realize the jar file names are not part of the original aar, but are really handled here. The fact that it's separated from the android.m4 part doesn't help clarity. I think it would be less bad to extract the aar and do this munging from configure itself. Which would have the side effect of making the extraction happen even less often than during build-backend. I also think those ANDROID_EXTRA_* variables would be better populated in Makefile.in/moz.build than here.

::: python/mozbuild/mozbuild/frontend/context.py:737
(Diff revision 1)
> +        """, 'export'),

This variable itself doesn't require the export rule to be triggered.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Very nice! I pulled the commits and did a couple of build with and without --disable-compile-environment and everything seems to work fine.

In addition to that I tried building with SDK 23: With just the "base" and "cast" AARs our build errors related to play services are down to just one (GooglePlayServicesUtil using Notification.setLatestEventInfo()) - Bug 1197147. Nice! Seems like most build errors where related to code that we didn't even need.

And then I updated my support library jar to 23 and I can still build successfully. :)

Comment 21

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/17805/#review15963

> I'm not sure that $(call callpath_template,-classpath,$(var)) is better than $(addprefix -classpath,$(call classpath_template,$(var))). It's shorter, for sure, but I'm not sure which one makes it clearer what the flag is. I think I tend towards the latter.

I agree.  I thought the conditional -- if the returned value is empty, we can't specify the prefix at all -- was a blocker, but addprefix handles that.  Changed.

> Why aren't $(default_classpath_jars) and $(default_bootclasspath_jars) in the dependencies?

Added.

> Removing this line is unrelated.

Yeah, just clean-up.  I'd like to leave it.

> Is there an open bug about making those Path instances?

No, so I filed Bug 1200876.

> I guess this is what you were asking for for a globally available variable. What we usually do for things like this is to use includes.

Actually I was looking for the ANDROID_STATIC_LIBRARIES to be global.  If we do that stuff at configure time, that problem goes away.

As for includes or similar, I think this would get cleaner if we had a unified Android APK declaration in moz.build, and then we could create these test APK declarations in reference to the master APK.  This is ugly in all build systems -- ours, Maven, Gradle.

Comment 22

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655078 [details]
MozReview Request: Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

Bug 1108782 - Part 1: straighten out Java classpaths. r?glandium

This commit is us getting out of our own way.  We were specifying
-classpath twice, once in $(JAVAC) and once in java-build.mk.  Only
the latter of these is active.  This a problem for ANDROID_EXTRA_JARS
-- those JARs should be on the classpath and input to $(DX) -- and
JARs that should be on the classpath but *not* input to $(DX).  This
commit removes the global flags to $(JAVAC) and adds
JAVA_{BOOT}CLASSPATH_JARS.  This required some hijinkery moving
wildcards to moz.build files, but everything seems to work.

As well as clarifying some parts of the build, part 2 uses this work
to modify the classpath.

Comment 23

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

This gets us a limited version of AAR support: we can consume static
AAR libraries, where here static does not refer to linking, but to
static assets that are fixed at build-backend time and not modified
(or produced) during the build.  This lets us pin our dependencies
(and move to Google's versioned Maven repository packages, away from
Google's unversioned ad-hoc packages).

By restricting to static AAR libraries, we avoid having to handle
truly complicated dependency trees, as changing parts of generated AAR
files require delicate rebuilding of the APKs (and internal libraries)
that depend on the AAR files.

It is possible that we will generate AARs in the tree at some time.
Right now, we don't do that, even for GeckoView: the AARs produced are
assembled as artifacts at package time and are intended for external
consumption.  We might want this for GeckoView and Fennec at some
time; we should consider using Gradle everywhere at that point.

The patch itself does the simplest possible thing (which has precedent
from Gradle and other build systems): it simply "explodes" the AAR
into the object directory and uses existing mechanisms to refer to the
exploded pieces.

There's a lot not to like in this approach, including:

* We need to manually reference internal AAR libs;
* I haven't separated the pinned version numbers out of configure.in.

However, it's closer to what we want than what we have!

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r?glandium

The new tar.xz file was produced by taking the existing file, removing
extras/*/support, and copying over the extras/*/m2repository from my
local machine.  These directories are all the same across all
installs, to the best of my knowledge.  I used |xz --compress| with no
additional options.

Comment 25

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #17)
> Note to self: check APK size difference (after Proguarding) using GPS -all
> and GPS {-base,-cast}.

From https://treeherder.mozilla.org/logviewer.html#?job_id=11078613&repo=try, I see 

Size of classes.dex: 	5037028 bytes 	
Size of fennec-43.0a1.en-US.android-arm.apk: 	41849559 bytes
Size of libxul.so: 	23169276 bytes

From https://treeherder.mozilla.org/logviewer.html#?job_id=4541412&repo=fx-team, I see
 	
Size of classes.dex: 	6154140 bytes 	
Size of fennec-43.0a1.en-US.android-arm.apk: 	43016340 bytes 	
Size of libxul.so: 	23169813 bytes

That's more than 1Mb shaved off our APK \o/

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8655078 [details]
MozReview Request: Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

https://reviewboard.mozilla.org/r/17805/#review16527

::: config/makefiles/java-build.mk:45
(Diff revisions 1 - 2)
> -classes.dex: $(ANDROID_CLASSPATH_JARS)
> -classes.dex: $(ANDROID_BOOTCLASSPATH_JARS)
> +classes.dex: $(default_classpath_jars) $(ANDROID_CLASSPATH_JARS)
> +classes.dex: $(default_bootclasspath_jars) $(ANDROID_BOOTCLASSPATH_JARS) $(ANDROID_EXTRA_JARS)

Maybe one per line?

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

https://reviewboard.mozilla.org/r/17807/#review16529

::: build/autoconf/android.m4:250
(Diff revisions 1 - 2)
> +  if ! test -e "$concat(ANDROID_, local_aar_var_base, _AAR)" ; then
> +    AC_MSG_ERROR([You must download the $1 AAR.  Run the Android SDK tool and install the Android and Google Support Repositories under Extras.  See https://developer.android.com/tools/extras/support-library.html for more info. (Looked for $concat(ANDROID_, local_aar_var_base, _AAR))])
> +  fi
> +  AC_SUBST(concat(ANDROID_, local_aar_var_base, _AAR))
> +  AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR)])

Considering how many times concat(ANDROID_, local_aar_var_base, _AAR) is used, you could define() a macro with that. Then the others would become e.g. concat(that_var, _LIB)

::: build/autoconf/android.m4:272
(Diff revisions 1 - 2)
> +  #   AC_MSG_RESULT([no])
> +  # else

Please remove

::: build/autoconf/android.m4:282
(Diff revisions 1 - 2)
> +  #   AC_MSG_RESULT([no])
> +  # else
> +  #   AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR_RES)])

Please remove

::: build/autoconf/android.m4:293
(Diff revisions 1 - 2)
> +  #   AC_MSG_RESULT([no])
> +  # else
> +  #   AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR_ASSETS)])

Please remove

::: build/autoconf/android.m4:298
(Diff revisions 1 - 2)
> +  AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR_ASSETS)])

I'm almost tempted to ask for a macro for those subtests that all look alike, but I won't block you on it.

::: mobile/android/base/moz.build:665
(Diff revisions 1 - 2)
> +        ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_APPCOMPAT_V7_AAR_RES']]
> +        ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_APPCOMPAT_V7_AAR_ASSETS']]

(this applies here and to all others similar changes)
You're now adding res and assets that you weren't adding before. Why the change?
What happens when the value of those _RES and _ASSETS variable is '' (which is valid per configure)?

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8656899 [details]
MozReview Request: Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

https://reviewboard.mozilla.org/r/18263/#review16531

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655078 [details]
MozReview Request: Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

This commit is us getting out of our own way.  We were specifying
-classpath twice, once in $(JAVAC) and once in java-build.mk.  Only
the latter of these is active.  This a problem for ANDROID_EXTRA_JARS
-- those JARs should be on the classpath and input to $(DX) -- and
JARs that should be on the classpath but *not* input to $(DX).  This
commit removes the global flags to $(JAVAC) and adds
JAVA_{BOOT}CLASSPATH_JARS.  This required some hijinkery moving
wildcards to moz.build files, but everything seems to work.

As well as clarifying some parts of the build, part 2 uses this work
to modify the classpath.

Comment 30

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

This gets us a limited version of AAR support: we can consume static
AAR libraries, where here static does not refer to linking, but to
static assets that are fixed at build-backend time and not modified
(or produced) during the build.  This lets us pin our dependencies
(and move to Google's versioned Maven repository packages, away from
Google's unversioned ad-hoc packages).

By restricting to static AAR libraries, we avoid having to handle
truly complicated dependency trees, as changing parts of generated AAR
files require delicate rebuilding of the APKs (and internal libraries)
that depend on the AAR files.

It is possible that we will generate AARs in the tree at some time.
Right now, we don't do that, even for GeckoView: the AARs produced are
assembled as artifacts at package time and are intended for external
consumption.  We might want this for GeckoView and Fennec at some
time; we should consider using Gradle everywhere at that point.

The patch itself does the simplest possible thing (which has precedent
from Gradle and other build systems): it simply "explodes" the AAR
into the object directory and uses existing mechanisms to refer to the
exploded pieces.  AARs have both required and optional components.
It's not easy to conditionally AC_SUBST variables, so optional
components will yield empty values.  The consuming build backend needs
to handle such empty values gracefully.

There's a lot not to like in this approach, including:

* We need to manually reference internal AAR libs;
* I haven't separated the pinned version numbers out of configure.in.

However, it's closer to what we want than what we have!

Comment 31

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8656899 [details]
MozReview Request: Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

The new tar.xz file was produced by taking the existing file, removing
extras/*/support, and copying over the extras/*/m2repository from my
local machine.  These directories are all the same across all
installs, to the best of my knowledge.  I used |xz --compress| with no
additional options.

Comment 32

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #27)
> Comment on attachment 8655079 [details]
> MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure
> time. r?glandium
> 
> https://reviewboard.mozilla.org/r/17807/#review16529
> 
> ::: build/autoconf/android.m4:250
> (Diff revisions 1 - 2)
> > +  if ! test -e "$concat(ANDROID_, local_aar_var_base, _AAR)" ; then
> > +    AC_MSG_ERROR([You must download the $1 AAR.  Run the Android SDK tool and install the Android and Google Support Repositories under Extras.  See https://developer.android.com/tools/extras/support-library.html for more info. (Looked for $concat(ANDROID_, local_aar_var_base, _AAR))])
> > +  fi
> > +  AC_SUBST(concat(ANDROID_, local_aar_var_base, _AAR))
> > +  AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR)])
> 
> Considering how many times concat(ANDROID_, local_aar_var_base, _AAR) is
> used, you could define() a macro with that. Then the others would become
> e.g. concat(that_var, _LIB)

Good idea.  I've done this.

> ::: build/autoconf/android.m4:272
> (Diff revisions 1 - 2)
> > +  #   AC_MSG_RESULT([no])
> > +  # else
> 
> Please remove
> 
> ::: build/autoconf/android.m4:282
> (Diff revisions 1 - 2)
> > +  #   AC_MSG_RESULT([no])
> > +  # else
> > +  #   AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR_RES)])
> 
> Please remove
> 
> ::: build/autoconf/android.m4:293
> (Diff revisions 1 - 2)
> > +  #   AC_MSG_RESULT([no])
> > +  # else
> > +  #   AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR_ASSETS)])
> 
> Please remove

Sorry!  I must have missed my clean-up pass.

> ::: build/autoconf/android.m4:298
> (Diff revisions 1 - 2)
> > +  AC_MSG_RESULT([$concat(ANDROID_, local_aar_var_base, _AAR_ASSETS)])
> 
> I'm almost tempted to ask for a macro for those subtests that all look
> alike, but I won't block you on it.

I did this, mostly to learn some autoconf/m4.

> ::: mobile/android/base/moz.build:665
> (Diff revisions 1 - 2)
> > +        ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_APPCOMPAT_V7_AAR_RES']]
> > +        ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_APPCOMPAT_V7_AAR_ASSETS']]
> 
> (this applies here and to all others similar changes)
> You're now adding res and assets that you weren't adding before. Why the
> change?
> What happens when the value of those _RES and _ASSETS variable is '' (which
> is valid per configure)?

First, not having assets was an oversight.  I think we id have res everywhere, though.  I added a comment to the commit message about what happens when these variables are empty: the build system has to handle that gracefully.  That could mean making things conditional, or not using empty variables.  I see no way around this, since conditional AC_SUBST is not possible, IIUC.

Comment 33

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

This gets us a limited version of AAR support: we can consume static
AAR libraries, where here static does not refer to linking, but to
static assets that are fixed at build-backend time and not modified
(or produced) during the build.  This lets us pin our dependencies
(and move to Google's versioned Maven repository packages, away from
Google's unversioned ad-hoc packages).

By restricting to static AAR libraries, we avoid having to handle
truly complicated dependency trees, as changing parts of generated AAR
files require delicate rebuilding of the APKs (and internal libraries)
that depend on the AAR files.

It is possible that we will generate AARs in the tree at some time.
Right now, we don't do that, even for GeckoView: the AARs produced are
assembled as artifacts at package time and are intended for external
consumption.  We might want this for GeckoView and Fennec at some
time; we should consider using Gradle everywhere at that point.

The patch itself does the simplest possible thing (which has precedent
from Gradle and other build systems): it simply "explodes" the AAR
into the object directory and uses existing mechanisms to refer to the
exploded pieces.  AARs have both required and optional components.
It's not easy to conditionally AC_SUBST variables, so optional
components will yield empty values.  The consuming build backend needs
to handle such empty values gracefully.

There's a lot not to like in this approach, including:

* We need to manually reference internal AAR libs;
* I haven't separated the pinned version numbers out of configure.in.

However, it's closer to what we want than what we have!

Comment 34

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8656899 [details]
MozReview Request: Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

The new tar.xz file was produced by taking the existing file, removing
extras/*/support, and copying over the extras/*/m2repository from my
local machine.  These directories are all the same across all
installs, to the best of my knowledge.  I used |xz --compress| with no
additional options.

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

https://reviewboard.mozilla.org/r/17807/#review17137

::: build/mobile/robocop/moz.build:14
(Diff revision 4)
> +# if CONFIG['ANDROID_SUPPORT_V4_AAR']:
> +#     ANDROID_EXTRA_PACKAGES += ['android.support.v4']
> +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_RES']]
> +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_ASSETS']]
> +# if CONFIG['ANDROID_RECYCLERVIEW_V7_AAR']:
> +#     ANDROID_EXTRA_PACKAGES += ['android.support.v7.recyclerview']
> +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_RES']]
> +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]

Either remove the block or the # :)

::: mobile/android/base/moz.build:55
(Diff revision 4)
> +    ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]

You wrote "The consuming build backend needs to handle such empty values gracefully." in the commit message. The above is not safe. The comment about conditional AC_SUBST doesn't make much sense either. Empty value has essentially the same meaning as "didn't AC_SUBST". So you can protect all this with simple ifs.

Comment 36

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #35)
> Comment on attachment 8655079 [details]
> MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure
> time. r?glandium
> 
> https://reviewboard.mozilla.org/r/17807/#review17137
> 
> ::: build/mobile/robocop/moz.build:14
> (Diff revision 4)
> > +# if CONFIG['ANDROID_SUPPORT_V4_AAR']:
> > +#     ANDROID_EXTRA_PACKAGES += ['android.support.v4']
> > +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_RES']]
> > +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_ASSETS']]
> > +# if CONFIG['ANDROID_RECYCLERVIEW_V7_AAR']:
> > +#     ANDROID_EXTRA_PACKAGES += ['android.support.v7.recyclerview']
> > +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_RES']]
> > +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]
> 
> Either remove the block or the # :)

Gah!  That's twice, sorry.  A fold that I thought was a delete, not a comment.

> ::: mobile/android/base/moz.build:55
> (Diff revision 4)
> > +    ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]
> 
> You wrote "The consuming build backend needs to handle such empty values
> gracefully." in the commit message. The above is not safe. The comment about
> conditional AC_SUBST doesn't make much sense either. Empty value has
> essentially the same meaning as "didn't AC_SUBST". So you can protect all
> this with simple ifs.

There's no sense making things conditional if leaving them out will fail.  (Or worse, build and then fail at runtime, since the underlying resources are missing.)  In this case, this is guarded with CONFIG['ANDROID_RECYCLERVIEW_V7_AAR'].  If that exists, then we know (from the form of the AAR) that the _AAR_ASSETS should exist.  If it doesn't, the AAR file itself has changed and we should re-evaluate what to build.  This is clearly error prone, but it's stricter than making everything conditional.

As for the comment, I have considerable confusion around this.  There's a difference with AC_DEFINE with empty and no AC_DEFINE at all (I think?  I hope!).  The fact that it's not like that for AC_SUBST is surprising and I hope, some day, we get strict about CONFIG['UNKNOWN'] and make that actually error.  In any case, tell me what you want to land this patch and I will do it.

Comment 37

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #36)
> (In reply to Mike Hommey [:glandium] from comment #35)
> > Comment on attachment 8655079 [details]
> > MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure
> > time. r?glandium
> > 
> > https://reviewboard.mozilla.org/r/17807/#review17137
> > 
> > ::: build/mobile/robocop/moz.build:14
> > (Diff revision 4)
> > > +# if CONFIG['ANDROID_SUPPORT_V4_AAR']:
> > > +#     ANDROID_EXTRA_PACKAGES += ['android.support.v4']
> > > +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_RES']]
> > > +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_ASSETS']]
> > > +# if CONFIG['ANDROID_RECYCLERVIEW_V7_AAR']:
> > > +#     ANDROID_EXTRA_PACKAGES += ['android.support.v7.recyclerview']
> > > +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_RES']]
> > > +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]
> > 
> > Either remove the block or the # :)
> 
> Gah!  That's twice, sorry.  A fold that I thought was a delete, not a
> comment.
> 
> > ::: mobile/android/base/moz.build:55
> > (Diff revision 4)
> > > +    ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]
> > 
> > You wrote "The consuming build backend needs to handle such empty values
> > gracefully." in the commit message. The above is not safe. The comment about
> > conditional AC_SUBST doesn't make much sense either. Empty value has
> > essentially the same meaning as "didn't AC_SUBST". So you can protect all
> > this with simple ifs.
> 
> There's no sense making things conditional if leaving them out will fail. 
> (Or worse, build and then fail at runtime, since the underlying resources
> are missing.)  In this case, this is guarded with
> CONFIG['ANDROID_RECYCLERVIEW_V7_AAR'].  If that exists, then we know (from
> the form of the AAR) that the _AAR_ASSETS should exist.  If it doesn't, the
> AAR file itself has changed and we should re-evaluate what to build.  This
> is clearly error prone, but it's stricter than making everything conditional.

Nothing guarantees that _AAR_ASSETS exists if _AAR is set, since you made it _optional_:
  MOZ_ANDROID_AAR_OPTIONAL_COMPONENT(concat(local_aar_var, _ASSETS), concat(root, assets))

So you can very well have foo_AAR set and foo_AAR_ASSETS empty. Which is what I'm saying will not work properly.
 
> As for the comment, I have considerable confusion around this.  There's a
> difference with AC_DEFINE with empty and no AC_DEFINE at all (I think?  I
> hope!).  The fact that it's not like that for AC_SUBST is surprising and I
> hope, some day, we get strict about CONFIG['UNKNOWN'] and make that actually
> error.  In any case, tell me what you want to land this patch and I will do
> it.

AC_DEFINE works differently because there *is* an important distinction made between -DFOO='' and no -DFOO at all.

Comment 38

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #37)
> (In reply to Nick Alexander :nalexander from comment #36)
> > (In reply to Mike Hommey [:glandium] from comment #35)
> > > Comment on attachment 8655079 [details]
> > > MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure
> > > time. r?glandium
> > > 
> > > https://reviewboard.mozilla.org/r/17807/#review17137
> > > 
> > > ::: build/mobile/robocop/moz.build:14
> > > (Diff revision 4)
> > > > +# if CONFIG['ANDROID_SUPPORT_V4_AAR']:
> > > > +#     ANDROID_EXTRA_PACKAGES += ['android.support.v4']
> > > > +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_RES']]
> > > > +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_SUPPORT_V4_AAR_ASSETS']]
> > > > +# if CONFIG['ANDROID_RECYCLERVIEW_V7_AAR']:
> > > > +#     ANDROID_EXTRA_PACKAGES += ['android.support.v7.recyclerview']
> > > > +#     ANDROID_EXTRA_RES_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_RES']]
> > > > +#     ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]
> > > 
> > > Either remove the block or the # :)
> > 
> > Gah!  That's twice, sorry.  A fold that I thought was a delete, not a
> > comment.
> > 
> > > ::: mobile/android/base/moz.build:55
> > > (Diff revision 4)
> > > > +    ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]
> > > 
> > > You wrote "The consuming build backend needs to handle such empty values
> > > gracefully." in the commit message. The above is not safe. The comment about
> > > conditional AC_SUBST doesn't make much sense either. Empty value has
> > > essentially the same meaning as "didn't AC_SUBST". So you can protect all
> > > this with simple ifs.
> > 
> > There's no sense making things conditional if leaving them out will fail. 
> > (Or worse, build and then fail at runtime, since the underlying resources
> > are missing.)  In this case, this is guarded with
> > CONFIG['ANDROID_RECYCLERVIEW_V7_AAR'].  If that exists, then we know (from
> > the form of the AAR) that the _AAR_ASSETS should exist.  If it doesn't, the
> > AAR file itself has changed and we should re-evaluate what to build.  This
> > is clearly error prone, but it's stricter than making everything conditional.
> 
> Nothing guarantees that _AAR_ASSETS exists if _AAR is set, since you made it
> _optional_:
>   MOZ_ANDROID_AAR_OPTIONAL_COMPONENT(concat(local_aar_var, _ASSETS),
> concat(root, assets))
> 
> So you can very well have foo_AAR set and foo_AAR_ASSETS empty. Which is
> what I'm saying will not work properly.

Right, I agree.  What "guarantees" this is the form of this particular AAR file.  I have gone through and figured out which variables will exist and which won't.

This isn't strict or robust, but I don't care to make it so.  It has never been the case that we handled mal-formed library directories or missing fileds -- that is, if the old _LIB was non-existent, we would fail.  Now, if an expected piece is missing, we will fail.  Making this stringent is not worth the effort.  (The guards are there because we need to allow parsing on non-Android build configurations for tests.)

> > As for the comment, I have considerable confusion around this.  There's a
> > difference with AC_DEFINE with empty and no AC_DEFINE at all (I think?  I
> > hope!).  The fact that it's not like that for AC_SUBST is surprising and I
> > hope, some day, we get strict about CONFIG['UNKNOWN'] and make that actually
> > error.  In any case, tell me what you want to land this patch and I will do
> > it.
> 
> AC_DEFINE works differently because there *is* an important distinction made
> between -DFOO='' and no -DFOO at all.

Yes, I agree.  But, since the AC_SUBST for missing pieces will be there regardless of whether we ever expect it to be there, what should we do?  We can't be strict about the pieces, because of the guards and tests.  What I've written guards based on _AAR (existence of package) and then will fail if expected AAR pieces are missing.  That's a reasonable compromise, to my mind.

Comment 39

[Mike Hommey [:glandium]]

It might fail, but it will fail is a very non obvious way, because it will try to find assets in / instead of the the right path where they are supposed to be but aren't. It seems to me the error would be clearer if the assets directory was completely missing from the list of directory, rather than being a completely wrong directory. But maybe I'm wrong, and they're equally bad.

Comment 40

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #39)
> It might fail, but it will fail is a very non obvious way, because it will
> try to find assets in / instead of the the right path where they are
> supposed to be but aren't. It seems to me the error would be clearer if the
> assets directory was completely missing from the list of directory, rather
> than being a completely wrong directory. But maybe I'm wrong, and they're
> equally bad.

This one will fail 'cuz the absolute path "%" breaks at build-backend time.  In this case, I think all the paths here are actual Paths.

Of course, not all things are Paths, which will fail in silent ways.  But this patch makes the situation is strictly no worse than what we had before.

Comment 41

[Mike Hommey [:glandium]]

I'm sorry, but the following failure is not going to help anyone know what's wrong:

The error occurred while processing the following file:

    mobile/android/base/moz.build

The error was triggered on line nnn of this file:

    ANDROID_ASSETS_DIRS += ['%' + CONFIG['ANDROID_RECYCLERVIEW_V7_AAR_ASSETS']]

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ["ValueError: Path '' is not absolute\n"]

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655078 [details]
MozReview Request: Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

Bug 1108782 - Part 1: straighten out Java classpaths. r=glandium

This commit is us getting out of our own way.  We were specifying
-classpath twice, once in $(JAVAC) and once in java-build.mk.  Only
the latter of these is active.  This a problem for ANDROID_EXTRA_JARS
-- those JARs should be on the classpath and input to $(DX) -- and
JARs that should be on the classpath but *not* input to $(DX).  This
commit removes the global flags to $(JAVAC) and adds
JAVA_{BOOT}CLASSPATH_JARS.  This required some hijinkery moving
wildcards to moz.build files, but everything seems to work.

As well as clarifying some parts of the build, part 2 uses this work
to modify the classpath.

Comment 43

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

This gets us a limited version of AAR support: we can consume static
AAR libraries, where here static does not refer to linking, but to
static assets that are fixed at build-backend time and not modified
(or produced) during the build.  This lets us pin our dependencies
(and move to Google's versioned Maven repository packages, away from
Google's unversioned ad-hoc packages).

By restricting to static AAR libraries, we avoid having to handle
truly complicated dependency trees, as changing parts of generated AAR
files require delicate rebuilding of the APKs (and internal libraries)
that depend on the AAR files.

It is possible that we will generate AARs in the tree at some time.
Right now, we don't do that, even for GeckoView: the AARs produced are
assembled as artifacts at package time and are intended for external
consumption.  We might want this for GeckoView and Fennec at some
time; we should consider using Gradle everywhere at that point.

The patch itself does the simplest possible thing (which has precedent
from Gradle and other build systems): it simply "explodes" the AAR
into the object directory and uses existing mechanisms to refer to the
exploded pieces.

AARs have both required and optional components.  Each component is
defined with an expected and required flag. If a component is expected
and not present, or not expected and is present, an error is raised.
If the component is expected and present, autoconf's ifelse() macro is
used to define the relevant AAR_* component variables.  If the
component is not expected and not present, no action is taken.  A
consuming build backend therefore can guard all AAR_* component
variables with just the top-level AAR variable.

Many AAR files have empty assets/ directories.  This patch doesn't
explode empty assets/ directories, protecting against trivial changes
to AAR files that don't impact the build.

There's a lot not to like in this approach, including:

* We need to manually reference internal AAR libs;
* I haven't separated the pinned version numbers out of configure.in.

However, it's closer to what we want than what we have!

Comment 44

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8656899 [details]
MozReview Request: Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

Bug 1108782 - Part 3: Update releng tooltool manifests to use android-sdk-linux with extras/*/m2repository directories. r=glandium

The new tar.xz file was produced by taking the existing file, removing
extras/*/support, and copying over the extras/*/m2repository from my
local machine.  These directories are all the same across all
installs, to the best of my knowledge.  I used |xz --compress| with no
additional options.

Comment 45

[Mike Hommey [:glandium]]

Comment on attachment 8655079 [details]
MozReview Request: Bug 1108782 - Part 2: Explode AAR files at configure time. r?glandium

https://reviewboard.mozilla.org/r/17807/#review18007

::: build/autoconf/android.m4:286
(Diff revisions 4 - 5)
> -  MOZ_ANDROID_AAR_REQUIRED_COMPONENT(concat(local_aar_var, _LIB), concat(root, $1-$2-classes.jar))
> +  MOZ_ANDROID_AAR_COMPONENT(concat(local_aar_var, _LIB), concat(root, $1-$2-classes.jar), 1)

Considering you don't care about the actual value of the third argument, you might as well make it more verbose, like passing REQUIRED.

::: build/autoconf/android.m4:301
(Diff revisions 4 - 5)
> -    MOZ_ANDROID_AAR(mediarouter-v7, 22.2.1, android, com/android/support)
> +    MOZ_ANDROID_AAR(mediarouter-v7, 22.2.1, android, com/android/support, 1)

And here it could be REQUIRED_INTERNAL_IMPL, although that's a mouthful.

::: python/mozbuild/mozbuild/action/explode_aar.py:31
(Diff revisions 4 - 5)
> +        if not any(name for name in zf.namelist() if name.startswith('assets/') and name != 'assets/'):
> +            assets = mozpath.join(destdir, 'assets')
> +            if os.path.exists(assets):
> +                shutil.rmtree(assets)

This would feel less convoluted if you just tried to rmdir assets unconditionally, catching the exception for ENOTEMPTY and ENOENT.

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8396485db77e
https://hg.mozilla.org/integration/fx-team/rev/d30da7e9633b
https://hg.mozilla.org/integration/fx-team/rev/511e9b64bd6c

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dd9ad40d97f3

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8396485db77e
https://hg.mozilla.org/mozilla-central/rev/d30da7e9633b
https://hg.mozilla.org/mozilla-central/rev/511e9b64bd6c
https://hg.mozilla.org/mozilla-central/rev/dd9ad40d97f3

Comment 49

[Dominique Vincent [:domivinc]]

Hi Nick,

On ubuntu, I get the following error:
/mozilla-central/other-licenses/android/res_state.c:63:36: fatal error: sys/_system_properties.h: No such file or directory

This error is already reported in bug 1091452 and bug 1133459. Both bugs are old bugs and not yet fixed. 

The only way to get a build on my ubuntu machine is to fall back to the previous configuration in mozconfig:
 ac_add_options --with-android-ndk="$HOME/android/android-ndk-r8e"
 ac_add_options --with-android-sdk="$HOME/android/adt-bundle-linux/sdk/platforms/android-22"

and to comment the following lines in android.m4 file:

    #if test -e "$withval"/source.properties ; then
        #AC_MSG_ERROR([Including platforms/android-* in --with-android-sdk arguments is deprecated.  Use --with-android-sdk=$android_sdk_root.])
    #fi

I understand and share the aim of this kind of change, you wrote: "This should particularly improve life for new volunteers coming to Fennec and sporadic contributors outside of the main Fennec development team.".
But on the same email, you wrote: "in particular, this hasn't been tested on Linux. And there are a number of tickets blocked on this work."

So the current status of this push is totally different of the initial aim: 
1- the new volunteers cannot build Fennec on ubuntu,
2- current contributors are blocked and cannot work anymore with a fresh version of the code.

The message you unfortunately sent to ubuntu developers is "move to another open source projects ...".

Could I suggest to "test on Linux" before pushing such change?  It will be really nice for new and current volunteers!

Comment 50

[Nick Alexander :nalexander [he/him]]

(In reply to Dominique Vincent [:domivinc] from comment #49)
> Hi Nick,
> 
> On ubuntu, I get the following error:
> /mozilla-central/other-licenses/android/res_state.c:63:36: fatal error:
> sys/_system_properties.h: No such file or directory
> 
> This error is already reported in bug 1091452 and bug 1133459. Both bugs are
> old bugs and not yet fixed. 

Let's unpack some of this.  I'm going to focus on the technical before we talk about the social.

What is your mozconfig?  The two bugs you cite are specific to NDK r10c and building --with-android-version=21.  |mach bootstrap| installs r10e and we don't support only support Android version 9 in the NDK.  (These patches shouldn't touch any of this.)  We use r10e on the Linux builders, so I'm quite surprised that you're seeing a problem.

Now, the social: I have limited time, so I can't test every platform, and I definitely can't test every set of flags.  In this case, we're taking a completely broken tool (|mach bootstrap| hasn't worked for months) and dramatically improving it.  Sometimes, things will regress, and we appreciate when folks like yourself help us fix them.

Comment 51

[Nick Alexander :nalexander [he/him]]

Amogh: I think you're attached to the Tor project, which is generally interested in reproducible builds.  The work here could be useful to you folks.  I'd be happy to include a SHA256 sum of the consumed AAR files, so you knew the input dependencies were true at build time.  To a lesser degree, you could do the same for Bug 1204260: we could include a SHA256 sum of the Android SDK tools that we use.  It's harder there since the versions are less well defined.  Something to think about.

Comment 52

[Dominique Vincent [:domivinc]]

(In reply to Nick Alexander :nalexander from comment #50)
> (In reply to Dominique Vincent [:domivinc] from comment #49)
> > Hi Nick,
> What is your mozconfig?  The two bugs you cite are specific to NDK r10c and

I used to work with this mozconfig without any issues:
ac_add_options --with-android-ndk="$HOME/android/android-ndk-r8e"
ac_add_options --with-android-sdk="$HOME/android/adt-bundle-linux/sdk/platforms/android-22"

After a |mach bootstrap|, at the end of the process, the display asked me to change the mozconfig this way:
ac_add_options --with-android-ndk="/home/vincent/.mozbuild/android-ndk-r10e"
ac_add_options --with-android-sdk="/home/vincent/.mozbuild/android-sdk-linux"

And it doesn't work.

I tried to add "ac_add_options --with-android-version=21" it still doesn't work.

Comment 53

[amoghbl1]

Thanks for tagging me nalexander, this will be useful when we get to the stage of trying reproducible builds with Orfox.