Closed
Bug 892603
Opened 10 years ago
Closed 10 years ago
Remove export tier from mobile/android/branding
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla25
People
(Reporter: nalexander, Assigned: nalexander)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 7 obsolete files)
1.55 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
5.01 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
We have some outdated export tier stuff (http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/aurora/content/Makefile.in#24) in mobile/android/branding that I have patches to remove. Here's a place to put them!
Assignee | ||
Comment 1•10 years ago
|
||
http://tbpl.mozilla.org/?tree=Try&rev=80b1321a3c24
Assignee | ||
Comment 2•10 years ago
|
||
MOZ_APP_ICON was a Makefile dependency not mentioned elsewhere in the tree.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
The text `LINUX_BRANDING` does not appear in the tree. There are no references to `fennec_48x48.png` or `fennec_72x72.png` outside of mobile/android/base/Makefile.in, and that file explicitly copies the referenced files (so does not depend on the deleted export rules).
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 774932 [details] [diff] [review] Part 1: Remove unused MOZ_APP_ICON from mobile/android/base/Makefile.in. Review of attachment 774932 [details] [diff] [review]: ----------------------------------------------------------------- blassey, mfinkle: this is a series of clean-ups and minor tweaks to the Android build system in preparation for Bug 854530 and Bug 890534. The only part with substance is renaming the fennecYxZ files to match what Android expects. I think this will make it easier to install the resources using moz.build (and, in some future world, buck). These patches block a build system Q3 goal of removing the export tier altogether.
Attachment #774932 -
Flags: review?(blassey.bugs)
Attachment #774932 -
Flags: feedback?(mark.finkle)
Assignee | ||
Comment 10•10 years ago
|
||
> These patches block a build system Q3 goal of removing the export tier
> altogether.
I mis-spoke: of removing the /recursive traversal/ of the export tier.
Comment 11•10 years ago
|
||
Be careful with these branding files. They are used by l10n repacks. I've attempted to touch them before only to have things break in weird and mysterious ways. I haven't looked at the patches, but I suspect you are playing with fire.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #11) > Be careful with these branding files. They are used by l10n repacks. I've > attempted to touch them before only to have things break in weird and > mysterious ways. I haven't looked at the patches, but I suspect you are > playing with fire. How? Where? I get that l10n repacks are serious voodoo, and I've been bitten myself, but these are Android image resources not referenced anywhere else in the tree. I'm tired of living in fear of l10n repacks, so I'm going to move and break things.
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #1) > http://tbpl.mozilla.org/?tree=Try&rev=80b1321a3c24 This is green for non-Android, but red for Android because a hunk got re-applied during rebase. Let's try again: http://tbpl.mozilla.org/?tree=Try&rev=7763edf3c6b7
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #13) > (In reply to Nick Alexander :nalexander from comment #1) > > http://tbpl.mozilla.org/?tree=Try&rev=80b1321a3c24 > > This is green for non-Android, but red for Android because a hunk got > re-applied during rebase. Let's try again: > > http://tbpl.mozilla.org/?tree=Try&rev=7763edf3c6b7 Try was down; try: http://tbpl.mozilla.org/?tree=Try&rev=b2bc6161a3a5
Updated•10 years ago
|
Attachment #774932 -
Flags: review?(blassey.bugs) → review+
Updated•10 years ago
|
Attachment #774932 -
Flags: feedback?(mark.finkle) → feedback+
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 774934 [details] [diff] [review] Part 2: Set ICON_PATH* regardless of MOZ_APP_NAME. Realized I didn't set flags on all the patches. Sorry :(
Attachment #774934 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #774935 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #774936 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #774937 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #774938 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #774939 -
Flags: review?(blassey.bugs)
Comment 16•10 years ago
|
||
Comment on attachment 774938 [details] [diff] [review] Part 6: Rename branding resources. Review of attachment 774938 [details] [diff] [review]: ----------------------------------------------------------------- why?
Updated•10 years ago
|
Attachment #774937 -
Flags: review?(blassey.bugs) → review+
Comment 17•10 years ago
|
||
I think in general I need to know why you are making these changes (for each patch).
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #17) > I think in general I need to know why you are making these changes (for each > patch). Totally reasonable. These patches are steps on the path to making m/a/b build using moz.build. The substantive work is partially tracked by Bug 854530 and Bug 890534. In detail: Part 1: Remove unused MOZ_APP_ICON from mobile/android/base/Makefile.in. This is just cruft left over from... I don't even know what. Part 2: Set ICON_PATH* regardless of MOZ_APP_NAME. I think this is just wrong, although it won't be hit at present. m/a/b/Makefile.in won't build without ICON_PATH* set. It is legal to change MOZ_APP_NAME (to be different from "fennec"), so I think this will cause (very rare) custom builds to fail. Part 3: Remove unused LINUX_BRANDING_FILES from mobile/android/branding/**/Makefile.in. (3.72 KB, patch) These files are never used. Part 4: Remove branding/**/android-resources.mn. This bit of Makefile awfulness needs to go before we make it to the moz.build world. This was an expedient way to allow for branding to add arbitrary resources to the Fennec APK; it turns out nothing actually got added. And this method wasn't even used consistently (icon was special-cased), and wasn't general enough to support installing to res/drawable-*. Kill it with fire. Part 5: Make RES_DRAWABLE parallel to other RES_*. This is just common sense, and you've already reviewed it. Part 6: Rename branding resources. (8.00 KB, patch) This is controversial. While working on Bug 854530 (cleaning up the Android resource declarations), I found that general copy + renaming like in/A -> out/A' is not well supported anywhere in our build system. We do have good support for copying in/A -> out/A. This is anticipating that we won't want the mismatch in resource names, where we have content/fennec72x72.png that gets copied to res/drawable-mpdi and content/fennec144x144.png that gets copied to res/drawable-xxhdpi. I'm not a big fan of churn for churn's sake, but having parallel directory structures seems better than having tricky Makefile rules do the renaming. If and when we build with Buck or similar, renaming shenanigans are not going to be an option. Part 7: Remove empty Makefile.in files. (3.38 KB, patch) This is the win for the build system. Bug 892644 tracks removing recursive traversal of the export tier; making these changes eliminates some of the recursive export invocations for that tier. Happy to clarify further, or break this apart, as you see fit.
Flags: needinfo?(blassey.bugs)
Comment 19•10 years ago
|
||
Comment on attachment 774934 [details] [diff] [review] Part 2: Set ICON_PATH* regardless of MOZ_APP_NAME. Review of attachment 774934 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ -425,5 @@ > > -else > -ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon48.png > -ICON_PATH_HDPI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon64.png > -endif this is the the standard icon sizes for xul-based projects. Fennec uses additional icons that are the sized specifically for Android. That is why the there is a MOZ_APP_NAME special case for "fennec"
Attachment #774934 -
Flags: review?(blassey.bugs) → review-
Comment 20•10 years ago
|
||
Comment on attachment 774935 [details] [diff] [review] Part 3: Remove unused LINUX_BRANDING_FILES from mobile/android/branding/**/Makefile.in. Review of attachment 774935 [details] [diff] [review]: ----------------------------------------------------------------- the text LINUX_BRANDING doesn't need to appear in the tree for these exports to be used. The thing to look for is the file names (fennec_48x48.png and fennec_72x72.png) in the tree and any MACROS that contain them. Those don't appear to be used. But it would probably be better to reference them from the object directory than the source directory, but we don't need to do that now.
Attachment #774935 -
Flags: review?(blassey.bugs) → review+
Comment 21•10 years ago
|
||
Comment on attachment 774936 [details] [diff] [review] Part 4: Remove branding/**/android-resources.mn. Review of attachment 774936 [details] [diff] [review]: ----------------------------------------------------------------- This is more finkle's area of concern
Attachment #774936 -
Flags: review?(blassey.bugs) → review?(mark.finkle)
Comment 22•10 years ago
|
||
Comment on attachment 774938 [details] [diff] [review] Part 6: Rename branding resources. Review of attachment 774938 [details] [diff] [review]: ----------------------------------------------------------------- maybe I'm missing something, but I don't see how this improves anything
Attachment #774938 -
Flags: review?(blassey.bugs) → review-
Comment 23•10 years ago
|
||
Comment on attachment 774939 [details] [diff] [review] Part 7: Remove empty Makefile.in files. Review of attachment 774939 [details] [diff] [review]: ----------------------------------------------------------------- again, this is mfinkle's domain
Attachment #774939 -
Flags: review?(blassey.bugs) → review?(mark.finkle)
Updated•10 years ago
|
Attachment #774939 -
Flags: review?(mark.finkle) → review+
Comment 24•10 years ago
|
||
Comment on attachment 774936 [details] [diff] [review] Part 4: Remove branding/**/android-resources.mn. I couldn't find any place we were trying to use the favicon32.png directly from Android, so I guess it won't be missed.
Attachment #774936 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #19) > Comment on attachment 774934 [details] [diff] [review] > Part 2: Set ICON_PATH* regardless of MOZ_APP_NAME. > > Review of attachment 774934 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/Makefile.in > @@ -425,5 @@ > > > > -else > > -ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon48.png > > -ICON_PATH_HDPI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon64.png > > -endif > > this is the the standard icon sizes for xul-based projects. Fennec uses > additional icons that are the sized specifically for Android. That is why > the there is a MOZ_APP_NAME special case for "fennec" If MOZ_APP_NAME is not "fennec", the existing code won't compile because ICON_PATH_{X,XX}HDPI aren't defined. This ties in with the resource renaming that I haven't yet convinced you is valuable, so let me make my case again. The best way to support our existing branding, and custom branding, is for the contents of $(MOZ_BRANDING_DIRECTORY)/res to be merged into m/a/b/resources. (In $(OBJDIR), of course). No renaming, no special cases, no defines based on MOZ_APP_NAME. Just copying. This patch is a step toward that end, where "just copying" could be via INSTALL_TARGETS or via some as-yet-to-be-written moz.build structure. Would it help if I posted an additional patch showing how INSTALL_TARGETS would improve the current icon shenanigans? There are no build tools (our own INSTALL_TARGETS included) that support renaming resources the way we do. (I acknowledge there aren't many that support preprocessing the way we do, either, and that lack of tool support doesn't mean our approach is wrong -- but this time it is.)
Assignee | ||
Comment 26•10 years ago
|
||
Per IRC conversation with blassey, renaming $(MOZ_BRANDING_DIRECTORY)/content/* to match Android's res/ convention is not the right thing to do for (future) XUL apps. I'm going to split this ticket into a few different pieces, sometimes carrying r+ forward, to land the good bits.
Flags: needinfo?(blassey.bugs)
Assignee | ||
Comment 27•10 years ago
|
||
MOZ_APP_ICON was a Makefile dependency not mentioned elsewhere in the tree. mfinkle already gave this a positive review.
Attachment #774932 -
Attachment is obsolete: true
Attachment #774934 -
Attachment is obsolete: true
Attachment #774935 -
Attachment is obsolete: true
Attachment #774936 -
Attachment is obsolete: true
Attachment #774937 -
Attachment is obsolete: true
Attachment #774938 -
Attachment is obsolete: true
Attachment #774939 -
Attachment is obsolete: true
Attachment #778148 -
Flags: review?
Assignee | ||
Comment 28•10 years ago
|
||
The text `LINUX_BRANDING` does not appear in the tree. There are no references to `fennec_48x48.png` or `fennec_72x72.png` outside of mobile/android/base/Makefile.in, and that file explicitly copies the referenced files (so does not depend on the deleted export rules). This leaves the relevant Makefile.in files empty, so we can remove them. blassey and mfinkle already gave this a positive review, in the form of obsoleted Parts 3 and 7.
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 778148 [details] [diff] [review] Part 1: Remove unused MOZ_APP_ICON from mobile/android/base/Makefile.in. r=mfinkle Review of attachment 778148 [details] [diff] [review]: ----------------------------------------------------------------- Carrying forward the review from mfinkle. This depends on Bug 895670, which removes logo.png from the branding directory.
Attachment #778148 -
Flags: review? → review+
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 778150 [details] [diff] [review] Part 2: Remove unused LINUX_BRANDING_FILES from mobile/android/branding/**/Makefile.in. r=mfinkle Review of attachment 778150 [details] [diff] [review]: ----------------------------------------------------------------- Carrying forward the review from blassey and mfinkle. This depends on Bug 895670, which removes logo.png from the branding directory.
Attachment #778150 -
Flags: review+
Assignee | ||
Comment 31•10 years ago
|
||
Okay, I've broken this bug into pieces, stripped out the parts blassey pointed out weren't consistent for XUL apps, and filed Bug 895685 to track (part of) fixing mobile/android/base/Makefile.in for XUL apps. Assuming mfinkle is cool with Bug 895670, the remaining pieces will remove the recursive export tier bits in mobile/android/branding.
Assignee | ||
Comment 32•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70028dc9b0b
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla25
Comment 33•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a70028dc9b0b
Assignee: nobody → nalexander
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•