979388 - mobile/android/base/AndroidManifest.xml.in make deps are incorrect

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

09:02 esawin: nalexander: it's reproducible, modify |AndroidManifest.xml.in|, |mach build mobile/android/base| (https://pastebin.mozilla.org/4468047), then again |mach build mobile/android/base| (https://pastebin.mozilla.org/4468048).
09:02 esawin: nalexander: the first build does the right thing, the second rebuilds for some reason.
09:02 nalexander: esawin: hmm, I could imagine the deps are a little off.  Ta.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: 4468047.txt

First log.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: 4468048.txt

Second log.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Make aapt invocation rebuild R.java. r=glandium

What appears to happen is the following:

Touch a resource.  On the next build, .aapt.deps is stale, so aapt is
invoked, which generates R.java, and we touch .aapt.deps.

Now R.java depends on .aapt.deps, but this does not appear to force Make
to consider targets that depend on R.java to be stale.  A target that
depends on R.java (such as gecko-R.jar) itself compares timestamps and
finds that gecko-R.jar is newer than R.java (from the previous build),
and this comparison appears to happen before aapt is invoked.  So even
though .aapt.deps is seen to be stale, and by transitivity R.java is
stale, this does not mark gecko-R.jar as stale.  The timestamp check
between R.java and gecko-R.jar appears to happen *before* aapt is
invoked.

On the second build following the update, the R.java generated in the
previous build is newer than gecko-R.jar, triggering the observed
rebuild of gecko-R.jar.

This commit forces the (timestamps of the) dependencies of .aapt.deps to
change on disk, which forces the appropriate gecko-R.jar rebuild.
What's strange about this solution is that it doesn't appear touch is
needed; any command whatsoever (including REPORT_BUILD) causes Make to
synchronize the timestamp check in the desired way.  I can't fathom if
this is a bug or a feature, or if I'm misunderstanding entirely.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Make aapt invocation rebuild R.java. r=glandium

This commit adds an empty recipe to dependencies of .aapt.deps, which
forces the appropriate gecko-R.jar rebuild.  This is because Make treats
targets with no recipe at all differently than targets with an empty
recipe, in a way that defeats our dependencies.

What appeared to be happening is the following:

Touch a resource.  On the next build, .aapt.deps is stale, so aapt is
invoked, which generates R.java, and we touch .aapt.deps.

Now R.java depends on .aapt.deps, but this does not appear to force Make
to consider targets that depend on R.java to be stale.  A target that
depends on R.java (such as gecko-R.jar) itself compares timestamps and
finds that gecko-R.jar is newer than R.java (from the previous build),
and this comparison appears to happen before aapt is invoked.  So even
though .aapt.deps is seen to be stale, and by transitivity R.java is
stale, this does not mark gecko-R.jar as stale.  The timestamp check
between R.java and gecko-R.jar appears to happen *before* aapt is
invoked.

On the second build following the update, the R.java generated in the
previous build is newer than gecko-R.jar, triggering the observed
rebuild of gecko-R.jar.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8390140 [details] [diff] [review]
Make aapt invocation rebuild R.java. r=glandium

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

::: mobile/android/base/Makefile.in
@@ +258,5 @@
>  # 4: directory to write R.java into.
>  # 5: directory to write R.txt into.
> +# We touch the target file before invoking aapt so that aapt's outputs
> +# are fresher than the target, preventing a subsequent invocation from
> +# thinking aapt's outputs are stale.

Please add a comment that this is safe because make removes the target file when one of the commands in the recipe fails.

Comment 6

[Nick Alexander :nalexander [he/him]]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/efe21df34742

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8390140 [details] [diff] [review]
Make aapt invocation rebuild R.java. r=glandium

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a.  I suppose you could say that Bug 978587 caused the issue, but that's not really true: that ticket is not yet on m-a, and it didn't cause the issue, it just makes it frequent.  But when I see results like https://tbpl.mozilla.org/php/getParsedLog.php?id=36244274&tree=Mozilla-Aurora, which could plausibly be an Android resource issue, I prefer to get the latest, most likely fixed code running in all places.

User impact if declined: none.  This is build only.

Testing completed (on m-c, etc.): it's been on m-c for several days, with no (apparent) fallout, and no recurrence of the Android resource problems we were seeing (at least, no recurrence that I know of).

Risk to taking this patch (and alternatives if risky): minor.  If the build breaks, the build breaks; and our tests are pretty good at failing (in hard to understand ways) when the Android resource problems do strike.

String or IDL/UUID changes made by this patch: none.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c20f0a77bbd7