Closed Bug 979388 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached file 4468047.txt
First log.
Attached file 4468048.txt
Second log.
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.
Attachment #8390101 - Flags: review?(mh+mozilla)
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.
Attachment #8390101 - Attachment is obsolete: true
Attachment #8390101 - Flags: review?(mh+mozilla)
Attachment #8390140 - Flags: review?(mh+mozilla)
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.
Attachment #8390140 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/fx-team/rev/efe21df34742
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/efe21df34742
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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.
Attachment #8390140 - Flags: approval-mozilla-aurora?
Attachment #8390140 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.