Clean up branding Makefiles

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
7 years ago
6 months ago

People

(Reporter: gps, Assigned: bokeefe)

Tracking

(Blocks 2 bugs)

Trunk
mozilla40
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

7 years ago
Posted patch Clean up Makefile.in, v1 (obsolete) — 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

7 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 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

7 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

7 years ago
Summary: Clean up browser/aurora/Makefile.in → Clean up branding Makefiles
Reporter

Comment 4

7 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

Updated

7 years ago
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+
Reporter

Comment 7

7 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9c80e2f95b
Target Milestone: --- → Firefox 18
Version: 9 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/8d9c80e2f95b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 789838
Reporter

Comment 9

7 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 → ---
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
Reporter

Updated

6 years ago
Assignee: gps → nobody
Assignee

Comment 11

4 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.
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+
Assignee

Comment 13

4 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

4 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
Flags: needinfo?(bokeefe)
Assignee

Comment 17

4 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
https://hg.mozilla.org/mozilla-central/rev/be97e101c20c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago4 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.