move RES_FILES to moz.build (java exports)

RESOLVED FIXED in mozilla27

Status

RESOLVED FIXED
5 years ago
7 months ago

People

(Reporter: joey, Assigned: nalexander)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Blocks: 847009
Blocks: 892644
(Assignee)

Comment 1

5 years ago
We want to do this as part of defining a build grammar for Java; see Bug 854530 and Bug 890534.
(Assignee)

Comment 2

5 years ago
Created attachment 815155 [details] [diff] [review]
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.
Attachment #815155 - Flags: review?(gps)
(Assignee)

Comment 3

5 years ago
Created attachment 815156 [details] [diff] [review]
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.
Attachment #815156 - Flags: review?(gps)
(Assignee)

Comment 4

5 years ago
Created attachment 815157 [details] [diff] [review]
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).
Attachment #815157 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Depends on: 923306
(Assignee)

Comment 5

5 years ago
A try build on top of some other pieces:

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

Comment 6

5 years ago
Man, I'm going insane.  This ticket does not depend on Bug 923306.
No longer depends on: 923306
(Assignee)

Comment 7

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

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

Comment 8

5 years ago
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.
Attachment #815155 - Flags: review?(gps) → review+

Comment 9

5 years ago
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?
Attachment #815156 - Flags: review?(gps) → review+
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.
Attachment #815157 - Flags: review?(gps) → review+
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
(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 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.
Attachment #815155 - Flags: feedback-
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.
Attachment #815157 - Flags: feedback-
https://hg.mozilla.org/mozilla-central/rev/7bbd04f25698
https://hg.mozilla.org/mozilla-central/rev/b63f1891d3e9
https://hg.mozilla.org/mozilla-central/rev/e85fb9e5291a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Updated

5 years ago
Blocks: 928149
(Assignee)

Updated

5 years ago
Blocks: 928183
(Assignee)

Comment 17

5 years ago
Follow up to support the android-sync repository:

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

Updated

7 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.