Allow Android resource directories in $SRCDIR

RESOLVED FIXED in mozilla28

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

unspecified
mozilla28
All
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 1 obsolete attachment)

2.87 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
12.67 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
2.30 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
50.79 KB, patch
glandium
: review+
gps
: review+
Details | Diff | Splinter Review
5.71 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
17.51 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
7.23 KB, patch
glandium
: review+
Details | Diff | Splinter Review
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.
Blocks: 923950
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?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
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.
Flags: needinfo?(l10n)
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 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.
Attachment #832674 - Flags: review?(mh+mozilla) → review-
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?
Attachment #832675 - Flags: review?(mh+mozilla)
Attachment #832675 - Flags: review?(gps)
Attachment #832675 - Flags: review+
Flags: needinfo?(mh+mozilla)
Attachment #832670 - Flags: review?(mark.finkle) → review+
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
Attachment #832672 - Flags: review?(mark.finkle) → review+
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.
(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.
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.
Attachment #832674 - Attachment is obsolete: true
Attachment #8333930 - Flags: review?(mark.finkle)
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.
Attachment #8333931 - Flags: review?(mark.finkle)
> 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 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.
Attachment #832675 - Flags: review?(gps) → review+
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.
Attachment #8333931 - Flags: review?(mark.finkle) → review+
Attachment #8333930 - Flags: review?(mark.finkle) → review+
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?
(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
Attachment #832669 - Flags: review?(mark.finkle) → review+
Rebased and folded, try looking good -- will land as soon as the trees open.

https://tbpl.mozilla.org/?tree=Try&rev=d32cedb2193d
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
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.
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.)
Flags: needinfo?(mh+mozilla)
(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
Removing this FORCE will bring us back the random incremental build failures because of missing strings, won't it?
I think the best option here is just to build gecko.ap_ from somewhere else that is built after mobile/android/base.
Flags: needinfo?(mh+mozilla)
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.
Attachment #8336516 - Flags: review?(mh+mozilla)
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.
Attachment #8336516 - Flags: review?(mh+mozilla) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.