Closed Bug 504953 Opened 10 years ago Closed 10 years ago

Clean up firefox branding in the build system

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla3.6a1

People

(Reporter: Dolske, Assigned: Dolske)

Details

Attachments

(2 files, 1 obsolete file)

The organization of the branding stuff irks me every time I touch it... The Firefox (release) bits are in /other-licenses/branding/firefox, and the generic (pre-release) branding is in /browser/branding/unofficial, but the nightly (Minefield) branding is scattered all over. This leads to a number of special cases in the build for nightly vs release/prerelease. And I'm always scared that make some change that works for nightly builds but will explode in release builds.

So, I've reorganized things to make this simpler. There should be no differences in the bits users get; this is all just build-side changes.

* All the scattered nightly bits move into /browser/branding/nightly (which is currently emptys sans 1 file). This tree now becomes identical (in layout) to /other-licenses/branding/firefox and /browser/branding/unofficial.

* MOZ_BRANDING_DIRECTORY is now always set. It defaults to /browser/branding/nightly. This also removes the need for the OFFICIAL_BRANDING and MOZ_USE_GENERIC_BRANDING hacks.

* Simplify $MOZ_BRANDING_DIRECTORY/Makefile.in. All it really does is copy files into $dist/branding/. There were a few cases of files being renamed as they're copied, I've moved that into browser/app/Makefile.in (so it's done in one place instead of each branding flavor).

* Fixed a few file permissions that didn't need to be executable (their different color in my shell's ls output was irritating me).
Attached patch Patch v.1 (obsolete) — Splinter Review
Builds on OSX, need to do some more testing on Windows/Linux.

* Gavin says old-homepage-default.properties is dead, so I've removed that too.
* "app.ico" doesn't seem to actually be used, so we no longer do the "cp firefox.ico branding/app.ico". Looks like the relevant code for Windows/OS2 just uses firefox.ico anyway.
Component: Theme → Build Config
QA Contact: theme → build.config
Attachment #389230 - Flags: review?(benjamin)
Comment on attachment 389230 [details] [diff] [review]
Patch v.1

Clobber builds on Windows (w/ official branding) and Mac (nightly) seem to be ok, so I'll go for review...
Comment on attachment 389230 [details] [diff] [review]
Patch v.1

I'm so happy! This is the way it was always supposed to be.
Attachment #389230 - Flags: review?(benjamin) → review+
Please remove old-homepage-default.properties is dead, please also remove it from browser/installer/{windows,unix}/packages-static too.
Attached patch Patch v.2Splinter Review
* Removes old-homepage-default.properties from packages-static
* Fixes browser/app/Makefile.in to work on Linux (remove a bogus nsinstall)
Attachment #389230 - Attachment is obsolete: true
Attachment #390096 - Attachment is patch: true
Attachment #390096 - Attachment mime type: application/octet-stream → text/plain
Pushed http://hg.mozilla.org/mozilla-central/rev/af248748c7ea
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This breaks l10n in weird ways.

Why are unofficial and nightly branding exposed to l10n now, and why without telling me?

Please revert the change to unofficial/locales/jar.mn, and make a similar hack to nightly/locales/jar.mn.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can you define "breaks l10n in weird ways"?

Why do you think nightlies/unofficial builds should be different than release versions? That's generally a bad idea.
You've added new modules to l10n without telling the l10n infrastructure. So all build infrastructure tells the localizers they're fine, just if they try to create an ad-hoc build, it'll fail over the missing branding strings.

We could properly add that new branding modules, but there was a conscious previous decision to not add unofficial branding and nightly branding to the localizable files. It's just adding noise and complexity in particular for the unofficial branding. Note that there is a lingering discussion on whether official branding should be localizable, too.

Reversing the decision to exclude modules without talking to the people involved is bad.
What was the reasoning behind that decision?  We absolutely should be able to build unofficial versions in any locale... I can't imagine why we wouldn't want to ensure that was possible.
(In reply to comment #10)
> What was the reasoning behind that decision? 

In case that question is directed at me:

There is little value in localizing "Minefield", "Shiretoko", or "Namaroka", and there is a potential danger in getting those names wrong.

Thus the decision to not expose those strings to l10n.

> We absolutely should be able to
> build unofficial versions in any locale... I can't imagine why we wouldn't want
> to ensure that was possible.

Sure, that's http://hg.mozilla.org/mozilla-central/rev/9b33be091bf5, just using a different setup in jar.mn.

Original bug was bug 440431.
(In reply to comment #10)
> We absolutely should be able to build unofficial versions in any locale...

That doesn't require the branding to be localizable, though, if we're willing to live with the lack of brand transcription in a few cases. We've lived with it so far, and it doesn't seem unreasonable for unofficial/nightly branding.

The changes to jar.mn can be reverted to address the l10n issue without a significant impact on this patch in general, right? Seems like we should just do that, unless I'm missing something. We can file a followup on making the other branding sets localizable if that's really desired.
Attached patch Followup fix v.1Splinter Review
Returns nightly and unofficial branding to en-US only.
Comment on attachment 392788 [details] [diff] [review]
Followup fix v.1

Thanks, r=me with one nit, make the comment in the nightly branding say Nightly instead of Unofficial?
Attachment #392788 - Flags: review+
Comment on attachment 392788 [details] [diff] [review]
Followup fix v.1

This is good to go.
Pushed the followup fix: http://hg.mozilla.org/mozilla-central/rev/415d1cdd5d7f
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 3.6a1 → mozilla3.6a1
You need to log in before you can comment on or make changes to this bug.