900522 - move RES_FILES to moz.build (java exports)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Nick Alexander :nalexander [he/him]]

We want to do this as part of defining a build grammar for Java; see Bug 854530 and Bug 890534.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Make ANDROID_RESFILES a moz.build-only variable. r=gps

This depends on Bug 923306, which I think is close to r=glandium.
Since this is more moz.build than Android, r?=gps.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Use ANDROID_RESFILES in mobile/android/base/moz.build. r=gps

This defines ANDROID_RESFILES in mobile/android/base/moz.build but
does not use the default processing from java-build.mk.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Add passthru ANDROID_GENERATED_RESFILES. r=gps

This defines all of the Android resources in moz.build files (although
some are still generated by mobile/android/base/Makefile.in).

Comment 5

[Nick Alexander :nalexander [he/him]]

A try build on top of some other pieces:

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Man, I'm going insane.  This ticket does not depend on Bug 923306.

Comment 7

[Nick Alexander :nalexander [he/him]]

Heh, good thing I tried that.  Here's with the trivial fix.

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

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 815155 [details] [diff] [review]
Part 1: Make ANDROID_RESFILES a moz.build-only variable. r=gps

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

I only glanced at the file lists. I trust things would break badly if you messed up.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 815156 [details] [diff] [review]
Part 2: Use ANDROID_RESFILES in mobile/android/base/moz.build. r=gps

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

There's no way I'm going to manually verify the list of files is consistent across the patch.

Did you diff the objdir or anything to verify output is consistent?

::: mobile/android/base/Makefile.in
@@ +594,5 @@
>  res/drawable-xxhdpi/icon.png: $(ICON_PATH_XXHDPI)
>  	$(NSINSTALL) -D res/drawable-xxhdpi
>  	cp $(ICON_PATH_XXHDPI) $@
>  
> +ANDROID_RESDIRS := $(subst resources/,res/,$(sort $(dir $(ANDROID_RESFILES))))

Please use a lowercase variable for local variables not used by rules.mk.

@@ +606,2 @@
>  	@echo "creating $@"
>  	$(NSINSTALL) $(subst res/,$(srcdir)/resources/,$@) $(dir $@)

Can we not use INSTALL_TARGETS for these?

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 815157 [details] [diff] [review]
Part 3: Add passthru ANDROID_GENERATED_RESFILES. r=gps

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

This is a very reluctant r+.

I would prefer we not move things to moz.build that still have their rules in individual Makefile.in files (only rules.mk). However, I know you are trying to enable IDE project generation for Fennec. That goal warrants bending the rules a bit.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 815156 [details] [diff] [review]
> Part 2: Use ANDROID_RESFILES in mobile/android/base/moz.build. r=gps
> 
> Review of attachment 815156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's no way I'm going to manually verify the list of files is consistent
> across the patch.
> 
> Did you diff the objdir or anything to verify output is consistent?

I've built with this many times, and verified all sorts of things by hand.  I'm not worried about busting the lists, since un-busting is trivial.

> ::: mobile/android/base/Makefile.in
> @@ +594,5 @@
> >  res/drawable-xxhdpi/icon.png: $(ICON_PATH_XXHDPI)
> >  	$(NSINSTALL) -D res/drawable-xxhdpi
> >  	cp $(ICON_PATH_XXHDPI) $@
> >  
> > +ANDROID_RESDIRS := $(subst resources/,res/,$(sort $(dir $(ANDROID_RESFILES))))
> 
> Please use a lowercase variable for local variables not used by rules.mk.

This is sneaky -- it is a variable used in rules.mk (well, java-build.mk), so this patch sets it, but then adds another variable to ignore it in rules.mk.  Let me post the follow-up which makes this less insane.

> @@ +606,2 @@
> >  	@echo "creating $@"
> >  	$(NSINSTALL) $(subst res/,$(srcdir)/resources/,$@) $(dir $@)
> 
> Can we not use INSTALL_TARGETS for these?

INSTALL_TARGETS doesn't handle nested directories, so we'd need 20 targets.  The follow-up will explain why I didn't do that.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 815157 [details] [diff] [review]
> Part 3: Add passthru ANDROID_GENERATED_RESFILES. r=gps
> 
> Review of attachment 815157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a very reluctant r+.
> 
> I would prefer we not move things to moz.build that still have their rules
> in individual Makefile.in files (only rules.mk). However, I know you are
> trying to enable IDE project generation for Fennec. That goal warrants
> bending the rules a bit.

I hear that.  I have a follow-up that installs the ANDROID_RESFILES using manifests, which is why I didn't go the final mile and build rules.mk support for mobile/android/base's resources.  (It's hard to do in make!)

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 815155 [details] [diff] [review]
Part 1: Make ANDROID_RESFILES a moz.build-only variable. r=gps

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

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +75,5 @@
> +        """Android resource files.
> +
> +        This variable contains a list of files to package into a 'res'
> +        directory and merge into an APK file.
> +        """, 'libs'),

config/makefiles/java-build.mk says it's export, not libs.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 815157 [details] [diff] [review]
Part 3: Add passthru ANDROID_GENERATED_RESFILES. r=gps

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

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +77,5 @@
> +        This variable contains a list of files that are expected to be
> +        generated (often by preprocessing) into a 'res' directory as
> +        part of the build process, and subsequently merged into an APK
> +        file.
> +        """, 'libs'),

export, not libs.

Comment 15

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/7bbd04f25698
https://hg.mozilla.org/integration/fx-team/rev/b63f1891d3e9
https://hg.mozilla.org/integration/fx-team/rev/e85fb9e5291a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7bbd04f25698
https://hg.mozilla.org/mozilla-central/rev/b63f1891d3e9
https://hg.mozilla.org/mozilla-central/rev/e85fb9e5291a

Comment 17

[Nick Alexander :nalexander [he/him]]

Follow up to support the android-sync repository:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0aa048f0c2f

Comment 18

[Carsten Book [:Tomcat]]

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