Closed
Bug 900522
Opened 9 years ago
Closed 9 years ago
move RES_FILES to moz.build (java exports)
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla27
People
(Reporter: joey, Assigned: nalexander)
References
(Blocks 4 open bugs)
Details
Attachments
(3 files)
11.23 KB,
patch
|
gps
:
review+
glandium
:
feedback-
|
Details | Diff | Splinter Review |
67.76 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
6.15 KB,
patch
|
gps
:
review+
glandium
:
feedback-
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•9 years ago
|
Blocks: nomakefiles
Assignee | ||
Comment 1•9 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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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 | ||
Comment 5•9 years ago
|
||
A try build on top of some other pieces: https://tbpl.mozilla.org/?tree=Try&rev=b82caf2d26f2
Assignee | ||
Comment 6•9 years ago
|
||
Man, I'm going insane. This ticket does not depend on Bug 923306.
No longer depends on: 923306
Assignee | ||
Comment 7•9 years ago
|
||
Heh, good thing I tried that. Here's with the trivial fix. https://tbpl.mozilla.org/?tree=Try&rev=ee0213b5b1ec
Comment 8•9 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•9 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 10•9 years ago
|
||
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•9 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•9 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 13•9 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]: ----------------------------------------------------------------- ::: 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 14•9 years ago
|
||
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-
Assignee | ||
Comment 15•9 years ago
|
||
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
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 16•9 years ago
|
||
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Assignee | ||
Comment 17•9 years ago
|
||
Follow up to support the android-sync repository: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0aa048f0c2f
Updated•4 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•