540833 - make update-packaging failed finding correct directory due to MOZ_PKG_PRETTYNAMES

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

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.

Comment 2

[Mark Banner (:standard8)]

The current thinking is that this is due to brandName being set to "Thunderbird" in release_config.py. Why it only affects Mac though.....

Comment 3

[Robert Kaiser]

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

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 5

[Philippe M. Chiasson (:gozer)]

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

Comment 6

[Philippe M. Chiasson (:gozer)]

Attachment: l10n repack mac bustage fix for branded builds

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

Comment 7

[Robert Kaiser]

This patch sounds strange, you shouldn't need to touch mozilla/ in any way, IIRC, things work fine for Firefox branded as Namoroka, etc.

Comment 8

[Mark Banner (:standard8)]

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

Comment 10

[Robert Kaiser]

The analysis Ben did in bug 544928 could be helpful in actually solving this.

Comment 11

[[e-mail] (:bhearsum)]

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

Comment 12

[[e-mail] (:bhearsum)]

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

Comment 13

[[e-mail] (:bhearsum)]

I'm going to bisect and figure out when this broke.

Comment 14

[Robert Kaiser]

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.

Comment 15

[[e-mail] (:bhearsum)]

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

Comment 16

[[e-mail] (:bhearsum)]

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.

Comment 17

[[e-mail] (:bhearsum)]

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.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 20

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

standard8: ping on this review? We think we'll need this for the upcoming FF3.7a3 in order to do alpha->alpha updates.

Comment 21

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Needed before we can do alpha->alpha updates for Mac. (Does not block alpha->alpha updates for win32, linux).

Comment 22

[Damon Sicore (:damons)]

Does this block manual alpha->alpha updates or only automated?

Comment 23

[[e-mail] (:bhearsum)]

(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

Comment 24

[[e-mail] (:bhearsum)]

Any MARs on Mac, I mean.

Comment 25

[Damon Sicore (:damons)]

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 26

[[e-mail] (:bhearsum)]

Comment on attachment 424791 [details] [diff] [review]
l10n repack mac bustage fix for branded builds

setting nick as a reviewer at his request

Comment 27

[Mark Banner (:standard8)]

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.

Comment 28

[Benjamin Smedberg]

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?

Comment 29

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 30

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 31

[Benjamin Smedberg]

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?

Comment 32

[[e-mail] (:bhearsum)]

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

Comment 33

[Nick Thomas [:nthomas] (UTC+12)]

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 ?

Comment 34

[Mark Banner (:standard8)]

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

Comment 35

[Benjamin Smedberg]

Marking this fixed, open a new bug if necessary.