Closed Bug 892603 Opened 10 years ago Closed 10 years ago

Remove export tier from mobile/android/branding

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

Part 1: Remove unused MOZ_APP_ICON from mobile/android/base/Makefile.in. r=mfinkle
1.55 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
Part 2: Remove unused LINUX_BRANDING_FILES from mobile/android/branding/**/Makefile.in. r=mfinkle
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!
MOZ_APP_ICON was a Makefile dependency not mentioned elsewhere in the
tree.
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).
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)
> 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.
Blocks: 892644
No longer blocks: nomakefiles
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.
(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.
(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
(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
Attachment #774932 - Flags: review?(blassey.bugs) → review+
Attachment #774932 - Flags: feedback?(mark.finkle) → feedback+
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)
Attachment #774935 - Flags: review?(blassey.bugs)
Attachment #774936 - Flags: review?(blassey.bugs)
Attachment #774937 - Flags: review?(blassey.bugs)
Attachment #774938 - Flags: review?(blassey.bugs)
Attachment #774939 - Flags: review?(blassey.bugs)
Comment on attachment 774938 [details] [diff] [review]
Part 6: Rename branding resources.

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

why?
Attachment #774937 - Flags: review?(blassey.bugs) → review+
I think in general I need to know why you are making these changes (for each patch).
(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 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 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 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 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 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)
Attachment #774939 - Flags: review?(mark.finkle) → review+
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+
(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.)
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)
Depends on: 895635
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?
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.
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+
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+
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70028dc9b0b
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/a70028dc9b0b
Assignee: nobody → nalexander
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.