Closed Bug 786520 Opened 12 years ago Closed 10 years ago

Clean up branding Makefiles

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

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)

Attached patch Clean up Makefile.in, v1 (obsolete) (deleted) — 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)
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 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?
Attached patch Consolidate branding Makefile redundancy, v1 (obsolete) (deleted) — Splinter Review
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)
Summary: Clean up browser/aurora/Makefile.in → Clean up branding Makefiles
Attached patch Consolidate branding Makefile redundancy, v2 (obsolete) (deleted) — Splinter Review
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)
Blocks: 786538
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: 12 years ago
Resolution: --- → FIXED
Depends on: 789838
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 (...)
Depends on: 844336
No longer depends on: 844336
Assignee: gps → nobody
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.
Attachment #656305 - Attachment is obsolete: true
Attachment #8596767 - Attachment is obsolete: true
Attachment #8598642 - Flags: review+
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
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Blocks: 1229341
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 40 → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: