934646 - Allow Android resource directories in $SRCDIR

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

At the moment, we have some nasty code for generating Android resource directories in $OBJDIR.  This is fragile, always hurting us for no-op builds, and difficult to reason about for l10n repacks.

In addition, it's really nasty for integration with IDEs, since editing and building are opposed.

The Android platform now supports defining multiple resource directories and combining them in various ways.  IDEs support this.  To have sensible IDE builds, we need to align our build system with this approach.

Technically, this means that calls to $(AAPT) have the --auto-add-overlay flag added and multiple -S directories in the command line.  There are open questions about how to declare this in moz.build files, which I will elaborate on in this ticket.

Comment 1

[Nick Alexander :nalexander [he/him]]

I want to separate our Android resources into static and generated.  The static ones live at mobile/android/base/resources, the generated ones are built into $OBJDIR/mobile/android/base/res.  The idea is that the number of generated resources remains very low (at the moment, 5 files (strings.xml + 4 icon resolutions), plus possible strings.xml for languages).

I think we should stop listing static resources in ANDROID_RESFILES.  This installation of ANDROID_RESFILES into $OBJDIR/m/a/b/res is tricky to get consistent in make; we've seen many bugs because we need to delete res, most recently Bug 923950.  Instead, we should list an ANDROID_RESDIR in the $SRCDIR directly and make building with all the resources in that directory work.  Modifying individual resources in this ANDROID_RESDIR will trigger a build update, and it's easy to make /adding/ a new resource file to the directory trigger a build update.  It's not trivial to make /removing/ a resource file trigger a build update.  My perspective is that this isn't a big deal.  We rarely remove static resources, and building a few times with an extra resource ID in R.java file isn't usually an issue.  In any case, changes that trigger a new R.java are frequent, so these stale builds should be infrequent.  (Both on developer and buildbot machines.)  To summarize: adding a $SRCDIR resource directory should not cause CLOBBER-needing build problems.

Now, I'd also like to stop deleting $OBJDIR/mobile/android/base/res.  Changes to generated resource prereqs will still trigger a build update.  Adding a new generated resource via the build system will necessarily modify the build configuration, triggering a build update.  Adding a new resource "out of band", like the l10n repacks and multilocale builds do, will "pollute" the objdir.  I'm fine with that.  On buildbots, these repack builds are one offs; we don't expect to be able to build incrementally afterwards.  It's not reasonable to expect the build system to recover from folks monkeying with $OBJDIR to make one build do something different.  Finally, removing a resource from the build system will of course trigger a rebuild, but won't clear the $OBJDIR/m/a/b/res.  This is classic CLOBBER territory, and I'm fine with keeping it that way.  The number of times that the *set* of generated resources changes is extremely low (and should remain so).

glandium, what do you think?  axel, what am I missing about how l10n really works?

Comment 2

[Axel Hecht [:Pike]]

I'd personally like to keep repacks to package files of the original package as much as possible. We're already pretty brittle when it comes to repackaging builds that don't match your local tree, but I'd rather not make that more common still.

As additional context, we'd like to be able to create a smaller "mozilla-central for l10n" repo, which would just have English strings and some build foo to enable local test setups. Requiring android source code to do a local test build would make that project harder than it is today. And it's already oh-so-hard that I'm not moving forward on it. But I'd also hate to give up on it completely.

The other downside is when you actually hack on repack stuff, you need your own build that matches your hacked tree, at least more than you do today.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: 1.Bug_934646___Always_include_Crash_Reporter_resources_in_Android_APK__r_mfinkle.patch

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: 2.Bug_934646___Delete_unused_resources__r_mfinkle.patch

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: 3.Bug_934646___Replace_hard_coded_Android_resource_ID__r_mfinkle.patch

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: 4.Bug_934646___Pre__Rename_branding_resources__r_glandium.patch

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: 5.Bug_934646___Declare_Android_resource_directories_relative_to__SRCDIR__r_glandium.patch

Comment 8

[Nick Alexander :nalexander [he/him]]

mfinkle: 1-3 are trivial with (maybe?) some policy thrown in.

glandium: 4 is mechanical. 5 is the meat.  This already works well with an updated |mach projectify|.  There's a good chance that this will fix Bug 923950, since with this patch we won't be mucking about copying the icons from branding at all.

Comment 9

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=201d3337b1f9

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 832674 [details] [diff] [review]
4.Bug_934646___Pre__Rename_branding_resources__r_glandium.patch

Review of attachment 832674 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is doing remove/add. It should be doing moves.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 832675 [details] [diff] [review]
5.Bug_934646___Declare_Android_resource_directories_relative_to__SRCDIR__r_glandium.patch

Review of attachment 832675 [details] [diff] [review]:
-----------------------------------------------------------------

I like it very much. BUT, I'd like another pair of eyes taking a look.

::: mobile/android/base/moz.build
@@ +356,5 @@
>  
> +ANDROID_RES_DIRS += [
> +    SRCDIR + '/resources',
> +    TOPSRCDIR + '/' + CONFIG['MOZ_BRANDING_DIRECTORY'] + '/res',
> +    OBJDIR + '/res',

'res' should work, right?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 832672 [details] [diff] [review]
3.Bug_934646___Replace_hard_coded_Android_resource_ID__r_mfinkle.patch

I'm assuming this passed Try

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 832669 [details] [diff] [review]
1.Bug_934646___Always_include_Crash_Reporter_resources_in_Android_APK__r_mfinkle.patch

This one is a bit slippery. I'm worried we start a trend of just adding resources without considering conditional safeguards. CrashReporter isn't a big deal, since we ship it almost unconditionally. But will we for GeckoView? and what about larger features that we what to add conditional safeguards for disabling on Aurora or higher?

Perhaps we need this complication as an example for the future. Cargo culting is real.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 832669 [details] [diff] [review]
> 1.
> Bug_934646___Always_include_Crash_Reporter_resources_in_Android_APK__r_mfinkl
> e.patch
> 
> This one is a bit slippery. I'm worried we start a trend of just adding
> resources without considering conditional safeguards. CrashReporter isn't a
> big deal, since we ship it almost unconditionally. But will we for
> GeckoView? and what about larger features that we what to add conditional
> safeguards for disabling on Aurora or higher?

This one is slippery, I agree.  It is a policy change that I thought should run through you.

As for GeckoView, that's trickier.  I don't understand how the existing code base and resource set will be split so I don't have a clear vision for this.

> Perhaps we need this complication as an example for the future. Cargo
> culting is real.

You don't need to tell me :)  I think the right thing to do is to conditionally add additional resource directories.  I anticipate few features that will need preprocessed resources and additional source resource directories are cheap and pretty clear.  I'll post a version that does this and see how it looks.

As a note: in IDEs, I'm planning to add labelling so the Fennec project will include subprojects like FennecGeneratedResources, FennecBrandingResources, and FennecCrashReporterResources.  Verbose yes, but clear and consistent when we have lots of projects.

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: Bug_934646___To_be_folded__Rename_branding_resources__r_glandium.patch

Did I mention I hate hg?  Here's an updated patch that uses `hg mv`.  Anybody can review this, so giving it to finkle since he's going to review another piece of this.  Mike, Greg -- whoever gets here first can review this.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Bug_934646___To_be_folded__Don_t_always_ship_crashreporter_resources_.patch

Mark: this will get folded into the main patch, since it busts bisect.

I chose crashreporter/ over crash_reporter/ by the 100:1 ratio of mxr hits.
 
We could follow-up by moving CrashReporter.java into crashreporter.

Comment 17

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=a76428ac0878

Comment 18

[Nick Alexander :nalexander [he/him]]

> I like it very much. BUT, I'd like another pair of eyes taking a look.

Yay!
 
> ::: mobile/android/base/moz.build
> @@ +356,5 @@
> >  
> > +ANDROID_RES_DIRS += [
> > +    SRCDIR + '/resources',
> > +    TOPSRCDIR + '/' + CONFIG['MOZ_BRANDING_DIRECTORY'] + '/res',
> > +    OBJDIR + '/res',
> 
> 'res' should work, right?

Now, yes; but when this gets transitioned to moz.build, everything will be relative to SRCDIR so I'd rather call out the explicit directories.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 832675 [details] [diff] [review]
5.Bug_934646___Declare_Android_resource_directories_relative_to__SRCDIR__r_glandium.patch

Review of attachment 832675 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: config/config.mk
@@ +34,5 @@
>  # present. If they are, this is a violation of the separation of
>  # responsibility between Makefile.in and mozbuild files.
>  _MOZBUILD_EXTERNAL_VARIABLES := \
>    ANDROID_GENERATED_RESFILES \
> +  ANDROID_RES_DIRS \

We should add ANDROID_RESFILES to the deprecated variables list.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8333931 [details] [diff] [review]
Bug_934646___To_be_folded__Don_t_always_ship_crashreporter_resources_.patch

I assume an hg move would be better, if possible, but I suppose the image and layout are very simple anyway.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 832669 [details] [diff] [review]
1.Bug_934646___Always_include_Crash_Reporter_resources_in_Android_APK__r_mfinkle.patch

Do we still need this one?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Comment on attachment 832669 [details] [diff] [review]
> 1.
> Bug_934646___Always_include_Crash_Reporter_resources_in_Android_APK__r_mfinkl
> e.patch
> 
> Do we still need this one?

Nick said the later patches are on top of it

Comment 23

[Nick Alexander :nalexander [he/him]]

Rebased and folded, try looking good -- will land as soon as the trees open.

https://tbpl.mozilla.org/?tree=Try&rev=d32cedb2193d

Comment 24

[Nick Alexander :nalexander [he/him]]

Pushed with a clobber:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd946b1f29b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae3d82562fa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a829f4d2584a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd8ff07e16f

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5dd946b1f29b
https://hg.mozilla.org/mozilla-central/rev/4ae3d82562fa
https://hg.mozilla.org/mozilla-central/rev/a829f4d2584a
https://hg.mozilla.org/mozilla-central/rev/6bd8ff07e16f

Comment 26

[Carsten Book [:Tomcat]]

had to backout this change 

remote: Inserted into the pushlog db successfully.
remote: You can view your changes at the following URLs:
remote:   https://hg.mozilla.org/mozilla-central/rev/b5413f07e669
remote:   https://hg.mozilla.org/mozilla-central/rev/7f841e04273d
remote:   https://hg.mozilla.org/mozilla-central/rev/597287004ff5

because of the suspicion this broke the Android Nightly Builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=30828286&tree=Mozilla-Central

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #26)
> had to backout this change 

note also backed this out from the integration trees just to avoid conflicts etc

Comment 28

[Nick Alexander :nalexander [he/him]]

Initial investigation shows that this is because the patch series made $OBJDIR/res/values/strings.xml have proper dependencies by FORCEing |make -C locales| during the build.  This is because locales/Makefile.in actually writes strings.xml, and therefore only it has full knowledge of the dependencies of strings.xml.

See https://hg.mozilla.org/mozilla-central/diff/a829f4d2584a/mobile/android/base/Makefile.in#l1.75

Apparently that doesn't work during an l10n repack.  That is, l10n repacks explicitly build gecko.ap_ but don't have enough information to ensure the constituent parts are fresh.

I can think of a few ways around this:

1. Don't FORCE strings.xml to be fresh.  This defeats the purpose of the build system.

2. Don't build strings.xml during an l10n repack.  There's precedent for this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/Makefile.in#59

What's weird is that we FORCE strings.xml from locales when we're repacking, and this suggestion would *not* FORCE strings.xml from base.  So fragile!

3. Be more clever about building strings.xml during an l10n repack.  This seems hard, since there are lots of delicate assumptions about configure, build, configure, build cycles.

Comment 29

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=4092bd6fd9e3

Comment 30

[Nick Alexander :nalexander [he/him]]

glandium: I see no way to make progress on this before I go on PTO Friday without just trying to make 1. above work, land on central with clobber, and back out if necessary.  Sigh.

r?  Only change is not FORCE-ing res/values/strings.xml at

https://hg.mozilla.org/try/rev/7f83faaa60f7#l10.75

(I also used |hg mv| for the crashreporter resources, which I missed earlier.)

Comment 31

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #30)
> glandium: I see no way to make progress on this before I go on PTO Friday
> without just trying to make 1. above work, land on central with clobber, and
> back out if necessary.  Sigh.
> 
> r?  Only change is not FORCE-ing res/values/strings.xml at
> 
> https://hg.mozilla.org/try/rev/7f83faaa60f7#l10.75

Compare:

https://hg.mozilla.org/mozilla-central/rev/a829f4d2584a#l10.75

Comment 32

[Mike Hommey [:glandium]]

Removing this FORCE will bring us back the random incremental build failures because of missing strings, won't it?

Comment 33

[Mike Hommey [:glandium]]

I think the best option here is just to build gecko.ap_ from somewhere else that is built after mobile/android/base.

Comment 34

[Nick Alexander :nalexander [he/him]]

Attachment: Don't check resource dependencies in |make package|.

Comment 35

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8336516 [details] [diff] [review]
Don't check resource dependencies in |make package|.

Review of attachment 8336516 [details] [diff] [review]:
-----------------------------------------------------------------

This makes |make package| produce a gecko-nodeps.ap_ that explicitly does not check resource dependencies.

It applies on top of the original three commits, and will be folded into the topmost commit to preserve bisect.

This is working well for me locally with multi locale builds.  Not sure how it will break single locale builds, but they're already broken, so...

Before you ask, I relocated rules.mk so that mkdir_deps would be available.

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 8336516 [details] [diff] [review]
Don't check resource dependencies in |make package|.

Review of attachment 8336516 [details] [diff] [review]:
-----------------------------------------------------------------

This is quite horrible, but i don't have a better idea that doesn't involve rewriting the entire l10n-repack build system.

Comment 37

[Nick Alexander :nalexander [he/him]]

Try build:

https://tbpl.mozilla.org/?tree=Try&rev=fa7ddef1fb3d

Push with final patch reviewed by glandium folded into part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0d76fafd52
https://hg.mozilla.org/integration/mozilla-inbound/rev/710003215a5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8490d45f1525
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be250f7458f

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b0d76fafd52
https://hg.mozilla.org/mozilla-central/rev/710003215a5c
https://hg.mozilla.org/mozilla-central/rev/8490d45f1525
https://hg.mozilla.org/mozilla-central/rev/9be250f7458f