Closed Bug 786520 Opened 8 years ago Closed 5 years ago
Clean up branding Makefiles
I went into browser/aurora/Makefile.in to remove the one-off export:: rule. While I was there, I performed some general cleanup (:= and removed tabs). I have no clue how to test these changes.
Comment on attachment 656283 [details] [diff] [review] Clean up Makefile.in, v1 Just my luck I notice that all the branding Makefiles are nearly identical. I'm going to refactor this remove all the DRY violations.
Comment on attachment 656283 [details] [diff] [review] Clean up Makefile.in, v1 >diff --git a/browser/branding/aurora/Makefile.in b/browser/branding/aurora/Makefile.in >+INSTALL_TARGETS += BRANDING_FILES Shouldn't this be BRANDING_FILES_FILES? If you're going to do this one, can you do browser/branding/*/Makefile.in too?
Read commit message for info. Try at http://tbpl.mozilla.org/?tree=Try&rev=540203a7cd97 I'm not sure if this will have unintended consequences for downstream package builders. It works for Mozilla's needs, however.
Summary: Clean up browser/aurora/Makefile.in → Clean up branding Makefiles
I had a typo in the last past that try caught. This fixes it. While I was there I renamed some variables to reduce IRC harassment from dolske. http://tbpl.mozilla.org/?tree=Try&rev=84382401ca8a
Try run seemed happy. https://tbpl.mozilla.org/?tree=Try&rev=84382401ca8a
Comment on attachment 656305 [details] [diff] [review] Consolidate branding Makefile redundancy, v2 Review of attachment 656305 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #656305 - Flags: review?(ted.mielczarek) → review+
Target Milestone: --- → Firefox 18
Version: 9 Branch → Trunk
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Backed out for breaking l10n nightlies. https://hg.mozilla.org/mozilla-central/rev/fc32318f8a8b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 18 → ---
For the record, the backout breaks incremental builds: cp: `browser/branding/nightly/default16.png' and `../../../dist/branding/default16.png' are the same file (...)
I think this makes everything under $(DIST)/branding managed by an install manifest. I'm pretty sure this is l10n-friendly, because I wrote a comment about it in recursivemake.py. I also have a copy of l10n-central, so I suspect I tried it at some point. The main thing is that the locale makefiles (e.g. https://hg.mozilla.org/mozilla-central/file/0b202671c9e2/browser/locales/Makefile.in#l149) do `make -C <branding dir> export` and expect files to end up in the right places.
Assignee: nobody → bokeefe
Status: REOPENED → ASSIGNED
Attachment #8596767 - Flags: review?(mshal)
Comment on attachment 8596767 [details] [diff] [review] Install things to $(DIST)/branding from moz.build It seems a bit much to introduce a new variable & install manifest for just a handful of files, but it doesn't look like we have a generic way to just install things into dist/ yet (maybe bug 853594 would simplify this). So r+ for now on the grounds that it removes some Makefiles, but I hope we can improve this in the future. >diff --git a/browser/branding/aurora/moz.build b/browser/branding/aurora/moz.build Since all these moz.build files are identical, maybe they can just do 'include ../branding-common.mozbuild' or some such, and list the files in there. >+ # Also emit the necessary rules to create $(DIST/branding during partial $(DIST)/branding (missing ')')
Attachment #8596767 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #12) > >diff --git a/browser/branding/aurora/moz.build b/browser/branding/aurora/moz.build > > Since all these moz.build files are identical, maybe they can just do > 'include ../branding-common.mozbuild' or some such, and list the files in > there. That's a good idea. I left the DIRS assignments and the export of DIST_SUBDIR in the individual moz.build files, because export is already non-obvious enough without it being hidden in an include. > >+ # Also emit the necessary rules to create $(DIST/branding during partial > > $(DIST)/branding (missing ')') Bah. Fixed.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8148fcc121ec (the orange was from an earlier version of bug 1155816). That push doesn't have the branding-common.mozbuild change, but I tested locally that it doesn't break anything.
This appears to have caused Linux PGO build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=9381821&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/6efd5103d766
I pushed this to try by itself, and that came back green, so it's probably safe to reland this one. https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c30abd4a15 (that's really a linux64-pgo build)
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.