961339 - Minimal resource changes cause various robocop failures, requiring clobber

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Bug 917896, which contains only updated png files and a single .java addition to moz.build, had several robocop failures when pushed: https://tbpl.mozilla.org/?tree=Fx-Team&rev=a63deedeb4b7.

Try is all green, however, so the problem is likely due to a needed clobber: https://tbpl.mozilla.org/?tree=Try&rev=7e2e23d43969

A clobber shouldn't be necessary here.

Comment 1

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #0)
> Bug 917896, which contains only updated png files and a single .java
> addition to moz.build

That is, speaking in terms of resource changes. Also forgot to mention that a couple layout XML files were modified.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Fix generated/ source dependencies.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Fix generated/ source dependencies.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8362702 [details] [diff] [review]
Fix generated/ source dependencies.

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

This does a few things.  First, it prints when fennec_ids.txt gets generated.  That would have been useful when debugging, since it appeared from the build log to not be getting built at all.

Second, it fixes the proguard JAR dependencies.  Pretty sure that $(list): $(list) is valid, but extra eyes appreciated.  Since proguard is a global operation, the dependency set should be the complete graph, as this defines it.  Before, this was essentially a FORCE.

Finally, this adds $CURDIR to the generated source dependencies.  When you landed the changes to the preprocessor that introduced absolute paths, this didn't get fully updated.  java-build.mk used $^ as much as possible and so building found the files, but the dependencies busted since make treats foo and $(CURDIR)/foo differently.  Adding $CURDIR to the generated dependencies means writing $(CURDIR)/../R.java, which actually fixes the issue I think caused this ticket.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8362702 [details] [diff] [review]
Fix generated/ source dependencies.

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

::: mobile/android/base/Makefile.in
@@ +98,1 @@
>  	java -jar $(ANDROID_SDK_ROOT)/tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars $(subst ::,:,$(subst $(NULL) ,:,$(strip $(ALL_JARS)))) -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB)

That's going to run several instances of that java command concurrently.

$ cat > foo.mk <<EOF
all: a b

a b:
        @echo $@
        @touch a b
EOF
$ make -f foo.mk
a
$ rm a b
$ make -f foo.mk -j2
a
b

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1036,5 @@
>          if jar.sources:
>              backend_file.write('%s_JAVAFILES := %s\n' %
>                  (target, ' '.join(jar.sources)))
>          if jar.generated_sources:
> +            backend_file.write('%s_PP_JAVAFILES := $(addprefix $(CURDIR)/,%s)\n' %

Seems to me this should be done in config/makefiles/java-build.mk. That being said, I don't see what that's supposed to fix.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Fix generated/ source dependencies. r=glandium

Generated sources are listed like 'generated/FILE.java'.  Generated
sources are produced by PP_TARGETS, which generates the file
'$(CURDIR)/generated/FILE.java'.  Because Make interprets $(CURDIR)/foo
and foo differently, this means we need to depend on $(CURDIR)/* for all
generated sources.  Since R.java is both listed as a generated source,
but produced by aapt, we need any dependencies to be on
$(CURDIR)/.../R.java.

The change to java-build.mk includes $(CURDIR) in the dependencies for
generated sources.  The changes to Makefile.in includes $(CURDIR) in the
produced R.java files for Make.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8370202 [details] [diff] [review]
Fix generated/ source dependencies. r=glandium

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

Hi glandium: this moves the relevant change to java-build.mk.  Hopefully the commit comment explains what this intends to do.  I'm going to push Proguard dep changes as part of a different ticket.

Comment 8

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e038f308b8d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out because I'm pretty sure this is the cause of intermittent Android build failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e52a4ab158ee

https://tbpl.mozilla.org/php/getParsedLog.php?id=34486749&tree=Mozilla-Inbound

Comment 10

[Nick Alexander :nalexander [he/him]]

We're definitely not doing this, post https://bugzilla.mozilla.org/show_bug.cgi?id=1414415 \o/