Closed
Bug 786520
Opened 12 years ago
Closed 10 years ago
Clean up branding Makefiles
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox40 fixed)
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: gps, Assigned: bokeefe)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
bokeefe
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #656283 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 1•12 years ago
|
||
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.
Attachment #656283 -
Flags: review?(mh+mozilla)
Comment 2•12 years ago
|
||
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?
Reporter | ||
Comment 3•12 years ago
|
||
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.
Assignee: nobody → gps
Attachment #656283 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #656290 -
Flags: review?(ted.mielczarek)
Reporter | ||
Updated•12 years ago
|
Summary: Clean up browser/aurora/Makefile.in → Clean up branding Makefiles
Reporter | ||
Comment 4•12 years ago
|
||
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
Attachment #656290 -
Attachment is obsolete: true
Attachment #656290 -
Flags: review?(ted.mielczarek)
Attachment #656305 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 5•12 years ago
|
||
Try run seemed happy. https://tbpl.mozilla.org/?tree=Try&rev=84382401ca8a
Comment 6•12 years ago
|
||
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+
Reporter | ||
Comment 7•12 years ago
|
||
Target Milestone: --- → Firefox 18
Version: 9 Branch → Trunk
Comment 8•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 9•12 years ago
|
||
Backed out for breaking l10n nightlies.
https://hg.mozilla.org/mozilla-central/rev/fc32318f8a8b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 18 → ---
Comment 10•12 years ago
|
||
For the record, the backout breaks incremental builds:
cp: `browser/branding/nightly/default16.png' and `../../../dist/branding/default16.png' are the same file
(...)
Reporter | ||
Updated•11 years ago
|
Assignee: gps → nobody
Assignee | ||
Comment 11•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
(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.
Attachment #656305 -
Attachment is obsolete: true
Attachment #8596767 -
Attachment is obsolete: true
Attachment #8598642 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
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.
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
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
Flags: needinfo?(bokeefe)
Assignee | ||
Comment 17•10 years ago
|
||
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)
Flags: needinfo?(bokeefe)
Keywords: checkin-needed
Comment 18•10 years ago
|
||
Keywords: checkin-needed
Comment 19•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
Updated•6 years ago
|
Target Milestone: Firefox 40 → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•