Closed Bug 540833 Opened 15 years ago Closed 14 years ago

make update-packaging failed finding correct directory due to MOZ_PKG_PRETTYNAMES

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Version:
1.9.1 Branch
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: standard8, Unassigned)

References

Details

(Whiteboard: [tbtrunkneeded])

Attachments

(1 file)

l10n repack mac bustage fix for branded builds
4.69 KB, patch
benjamin
: review+
Details | Diff | Splinter Review 
We've just tried getting build automation to do a build of Lanikai Alpha 1. Windows and Linux worked fine, Mac did not.

To do this, we're setting:

MOZ_APP_NAME=thunderbird
MOZ_APP_DISPLAYNAME=Lanikai

When the build automation runs:

make -C objdir-tb/ppc/mozilla/tools/update-packaging

we're then getting:

mkdir -p ../../dist/update/mac/en-US/
MAR=/builds/slave/macosx_build/build/objdir-tb/ppc/mozilla/dist/host/bin/mar \
	  /builds/slave/macosx_build/build/mozilla/tools/update-packaging/make_full_update.sh \
	  "../../dist/update/mac/en-US//lanikai-3.1a1.complete.mar" \
	  "../../dist/universal/Lanikai/Lanikai.app"
/builds/slave/macosx_build/build/mozilla/tools/update-packaging/make_full_update.sh: line 42: pushd: ../../dist/universal/Lanikai/Lanikai.app: No such file or directory
make: *** [complete-patch] Error 1

That step is setting MOZ_PKG_PRETTYNAMES=1.

I can reproduce this on my self build with MOZ_PKG_PRETTYNAMES=1. If I don't set MOZ_PKG_PRETTYNAMES then the build works fine but gives us thunderbird-3.1a1.en-US.mac.complete.mar rather than lanikai-3.1a1.en-US.mac.complete.mar.
From what I've worked out so far, the error is either:

http://hg.mozilla.org/mozilla-central/annotate/b91227360383/toolkit/mozapps/installer/package-name.mk#l120

ifndef MOZ_PKG_APPNAME
MOZ_PKG_APPNAME = $(MOZ_APP_DISPLAYNAME)
endif

or in one of the lines starting at http://hg.mozilla.org/mozilla-central/annotate/b91227360383/tools/update-packaging/Makefile.in#l57

One of those options is causing the directory naming to be different:

../../dist/universal/Lanikai/Lanikai.app

versus

../../dist/universal/thunderbird/Lanikai.app

I'm not convinced which one is right or expected.
The current thinking is that this is due to brandName being set to "Thunderbird" in release_config.py. Why it only affects Mac though.....
(In reply to comment #2)
> The current thinking is that this is due to brandName being set to
> "Thunderbird" in release_config.py. Why it only affects Mac though.....

The reason sounds plausible, and why it affects only Mac is probably to be found somewhere in factory.py, where we probably are using the brandName to point to the right path, which is probably even the right thing to do when brandName is set correctly.
Given the hand-hack that we had to do here, do we still think this is a bug in core?  gozer, are you the right person to own this bug?
(In reply to comment #3)
> (In reply to comment #2)
> > The current thinking is that this is due to brandName being set to
> > "Thunderbird" in release_config.py. Why it only affects Mac though.....

I tried a new build with Lanakai in brandName of release_config, and it failed in exactly the same way, so that's not it.

> The reason sounds plausible, and why it affects only Mac is probably to be
> found somewhere in factory.py, where we probably are using the brandName to
> point to the right path, which is probably even the right thing to do when
> brandName is set correctly.

Actually, it looks like it's something that's broken in comm-central's build system, and that release automation/buildbot is not to blame.
When running the l10n repacks on Mac OS X for a branded build (Lanikai), I've run into bustage, attached patch fixes it, but not 100% certain it's a safe patch
Attachment #424791 - Flags: review?(bugzilla)
This patch sounds strange, you shouldn't need to touch mozilla/ in any way, IIRC, things work fine for Firefox branded as Namoroka, etc.
(In reply to comment #7)
> This patch sounds strange, you shouldn't need to touch mozilla/ in any way,
> IIRC, things work fine for Firefox branded as Namoroka, etc.

I'm certainly suspicious of us needing to do patches like that, I just haven't had time to examine in detail. One thing to note is that AFAIK Namoroka didn't do l10n builds of its alpha releases so there may be issues there that weren't picked up.
The analysis Ben did in bug 544928 could be helpful in actually solving this.
Just to keep everything in one place, here's my comment from bug 544928:
This used to work fine but broke sometime between the 3.6a1 and the 3.7a1
builds. MOZ_PKG_DIR is set to MOZ_APP_DISPLAYNAME for release builds (whereas
it's normally set to MOZ_APP_NAME). For alphas, MOZ_APP_DISPLAYNAME gets
changed to a code name. When the update-packaging target tried to run for
3.7a1, we had this error:
mkdir -p ../../dist/update/mac/en-US/
MAR=/builds/slave/macosx_build/build/obj-firefox/ppc/dist/host/bin/mar \
     
/builds/slave/macosx_build/build/tools/update-packaging/make_full_update.sh \
      "../../dist/update/mac/en-US//mozilladeveloperpreview-3.7a1.complete.mar"
\
     
"../../dist/universal/MozillaDeveloperPreview/MozillaDeveloperPreview.app"
/builds/slave/macosx_build/build/tools/update-packaging/make_full_update.sh:
line 42: pushd:
../../dist/universal/MozillaDeveloperPreview/MozillaDeveloperPreview.app: No
such file or directory
make: *** [complete-patch] Error 1

*But*, in dist/universal/firefox we had a MozillaDeveloperPreview.app. So it
seems like at some point 'make package' started putting the package in a dir
based on MOZ_APP_NAME rather than MOZ_APP_DISPLAYNAME.

The workaround used was to switch MOZ_APP_DISPLAYNAME with MOZ_PKG_DIR in
tools/update-packaging/Makefile.in
(In reply to comment #8)
> (In reply to comment #7)
> > This patch sounds strange, you shouldn't need to touch mozilla/ in any way,
> > IIRC, things work fine for Firefox branded as Namoroka, etc.
> 
> I'm certainly suspicious of us needing to do patches like that, I just haven't
> had time to examine in detail. One thing to note is that AFAIK Namoroka didn't
> do l10n builds of its alpha releases so there may be issues there that weren't
> picked up.

This is correct, but l10n is a red herring. The specific problem occurs when MOZ_APP_NAME and MOZ_APP_DISPLAYNAME differ, and you attempt to create a MAR on Mac.
I'm going to bisect and figure out when this broke.
Hmm, was the patch for http://hg.mozilla.org/mozilla-central/rev/42d7cb939283
somehow wrong or incomplete?
I just went through all patches since 3.6a1 that touched package-name.mk and
that one is the only one that looked suspicious.
(In reply to comment #14)
> Hmm, was the patch for http://hg.mozilla.org/mozilla-central/rev/42d7cb939283
> somehow wrong or incomplete?
> I just went through all patches since 3.6a1 that touched package-name.mk and
> that one is the only one that looked suspicious.

That seems more likely than any of the other patches I've seen....I'll look around that changeset first.
hg bisect points to http://hg.mozilla.org/mozilla-central/rev/b375e5cab4dc -- which is the changeset in which MOZ_PKG_PRETTYNAMES landed. So, this has been broken for a long time.

Ted suggested that getting flight.mk to pull in vars from package-names.mk instead of defining its own would be a good fix here. I'll give that a try.
Before I got a chance to really dive into this I was told that Alpha -> Alpha partials for Firefox have been deprioritized.  I won't have a chance to dig into this anytime soon.
No longer blocks: 543794
Adding [tb31needs], because this is still going to be a problem for Tb 3.1b2 if we ship it as Lanikai.  Ben, can I talk you into spending 5-10 mins to figure out how much work it's likely to be to do the right thing here?
Whiteboard: [tb31needs]
standard8: ping on this review? We think we'll need this for the upcoming FF3.7a3 in order to do alpha->alpha updates.
Needed before we can do alpha->alpha updates for Mac. (Does not block alpha->alpha updates for win32, linux).
blocking2.0: --- → ?
Does this block manual alpha->alpha updates or only automated?
(In reply to comment #22)
> Does this block manual alpha->alpha updates or only automated?

It blocks manual ones, too. We can't generate *any* MARs without this fixed
Any MARs on Mac, I mean.
Who actually needs to review, here?  standard8?  Is that the same person as in
the review request?  Just want to make sure the right person(s) are seeing the
request for review.
Comment on attachment 424791 [details] [diff] [review]
l10n repack mac bustage fix for branded builds

setting nick as a reviewer at his request
Attachment #424791 - Flags: review?(nrthomas)
So I'm probably not the right person to review. However, some comments:

1) no-one seems to know the original intention of how these were meant to be packaged i.e. which directories to be created.
2) I'm not convinced the patch is complete (especially for Firefox)/will work.
3) Ben's workaround in comment 11 may be better.

Unfortunately I've not got time to work on this at the moment, so it is probably better that somemone else looks at it.
Attachment #424791 - Flags: review?(bugzilla)
Comment on attachment 424791 [details] [diff] [review]
l10n repack mac bustage fix for branded builds

Hrm, currently MOZ_PKG_DIR = $(MOZ_APP_NAME) not $(MOZ_PKG_APPNAME)... is that difference consequential?
AIUI (and this is all for en-US), we call unify at the end of 'make build', and that puts the the universal app in
  $(DIST_UNI)/$(MOZ_PKG_APPNAME)/$(APPNAME)
via flight.mk line 112. Neglecting the DIST_UNI part, that expands to
  $(MOZ_APP_NAME)/$(MOZ_APP_DISPLAYNAME).app
That's using the definitions earlier in flight.mk, and I've added spaces to make it clear what comes each var.

For nightly branding we set 
  MOZ_APP_NAME    = firefox
  MOZ_APP_DISPLAYNAME = Minefield
in config/autoconf.mk, while for 3.7a2 we had
  MOZ_APP_NAME    = firefox
  MOZ_APP_DISPLAYNAME = MozillaDeveloperPreview

When we come to do 'make package' we end up at 
  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#172
and use $(PKG_DMG_SOURCE) as the location of the files for the dmg. A couple of lines earlier that's defined as $(STAGEPATH)$(MOZ_PKG_DIR), and that expands to
universal/$(MOZ_APP_NAME) via package-name.mk. So that lines up, although not without adding some vars to make it less obvious what's going on.

When creating the complete mar we expect the source files in
 PACKAGE_DIR	= $(PACKAGE_BASE_DIR)/universal/$(MOZ_PKG_APPNAME)/$(MOZ_APP_DISPLAYNAME).app
Here MOZ_PKG_APPNAME comes from package-name.mk and is MOZ_APP_NAME for nightlies and MOZ_APP_DISPLAYNAME for releases (where MOZ_PKG_PRETTYNAMES is set so that the binaries have the right name straight away). 

So using MOZ_PKG_DIR for update-packaging is correct. In a perfect world flight.mk would use the same variables too.
blocking2.0: ? → ---
blocking2.0: --- → ?
Attachment #424791 - Flags: review?(nrthomas) → review+
Comment on attachment 424791 [details] [diff] [review]
l10n repack mac bustage fix for branded builds

Checked in the m-c part to fix en-US builds:
  http://hg.mozilla.org/mozilla-central/rev/da1d4c2324a5

Locales are still outstanding.
Whiteboard: [tb31needs] → [tb32needs]
This bug has a blocking nomination but no owner, and I'm not sure what remains. Can we either close it or assign it to the person who cares?
(In reply to comment #30)
> Comment on attachment 424791 [details] [diff] [review]
> l10n repack mac bustage fix for branded builds
> 
> Checked in the m-c part to fix en-US builds:
>   http://hg.mozilla.org/mozilla-central/rev/da1d4c2324a5
> 
> Locales are still outstanding.

Do you recall what needs to be done, Nick?
Anything I knew in comment #29 has evaporated, but perhaps
 http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#147
just works because it calls tools/update-packaging/Makefile.in, which we fixed already.

In Firefox nightlies we end up calling
MAR=/builds/slave/mozilla-central-macosx-l10n-nightly/build/mozilla-central/dist/host/bin/mar \
	  ./make_full_update.sh \
	  "/builds/slave/mozilla-central-macosx-l10n-nightly/build/mozilla-central/browser/locales/../../dist/update//firefox-4.0b3pre.ku.mac.complete.mar" \
	  "/builds/slave/mozilla-central-macosx-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app"

vs this in a 4.0b2 release locale:
MAR=/builds/moz2_slave/macosx_repack/build/mozilla-1.9.2/dist/host/bin/mar \
	  ./make_full_update.sh \
	  "/builds/moz2_slave/macosx_repack/build/mozilla-1.9.2/browser/locales/../../dist/update/mac/or//firefox-3.6.8.complete.mar" \
	  "/builds/moz2_slave/macosx_repack/build/mozilla-1.9.2/browser/locales/../../dist/l10n-stage/Firefox/Firefox.app"

Thunderbird dudes, have you done a localized unofficial branding build since 2010-03-11 ?
(In reply to comment #33)
> Thunderbird dudes, have you done a localized unofficial branding build since
> 2010-03-11 ?

Nope, and I believe our next alphas will be non-localized as well. I guess this no-longer blocks 2.0. We could possibly close it for now due to "not needed" and file a new one if the need for localized unofficial branding comes up again.
Marking this fixed, open a new bug if necessary.
Status: NEW → RESOLVED
blocking2.0: ? → ---
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [tb32needs] → [tbtrunkneeded]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: