make it possible to create files in the final place for releases

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: kairo, Assigned: bhearsum)

Tracking

({fixed1.9.1})

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [hg-automation])

Attachments

(6 attachments, 6 obsolete attachments)

17.31 KB, patch
ted
: review+
Details | Diff | Splinter Review
13.60 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.86 KB, patch
Details | Diff | Splinter Review
6.69 KB, patch
Details | Diff | Splinter Review
10.96 KB, patch
ted
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.25 KB, patch
ted
: review+
Details | Diff | Splinter Review
Reporter

Description

11 years ago
The Firefox release process places files with "pretty" names into platform and locale subdirectories, we should create file with those names and directories right away in the build system package process.
Reporter

Comment 1

11 years ago
This patch adds a MOZ_PKG_PRETTYNAMES variable that enables using the release-style "pretty" package names. As those require special subdirectories, we also need to add better handling of package directories to the build system - in many places this only parameterizes previously hardcoded directories.

For achieving the complete set of namings we e.g. have for Shiretoko Alpha 2, we'd need the mozconfig (or the environment) to contain the following settings:
-------------------------
MOZ_PKG_PRETTYNAMES=1
MOZ_PKG_VERSION=alpha2
MOZ_PKG_LONGVERSION=Alpha 2
-------------------------
(the "Shiretoko" name is known to the build from MOZ_APP_DISPLAYNAME already)

For a Firefox 3.1 Beta 1, we'd probably need something like:
-------------------------
MOZ_PKG_PRETTYNAMES=1
MOZ_PKG_VERSION=3.1beta1
MOZ_PKG_LONGVERSION=3.1 Beta 1
-------------------------

For a Firefox 3.1 final, we can go with:
-------------------------
MOZ_PKG_PRETTYNAMES=1
-------------------------
as both version strings then will just be set to the internally known Firefox version number.


The one part I haven't figured out yet is partial update naming, I can't really figure out how this is set right now, what I see in update-packaging/Makefile.in sounds a bit strange to me.
Attachment #337071 - Flags: review?(ted.mielczarek)
Reporter

Comment 2

11 years ago
Two additional comments:
1) This needs testing, I only have Linux here and haven't tried all the cases even there yet.
2) Checking in the patch this way shouldn't break Thunderbird or SeaMonkey currently, but they should be fixed up the same way as browser/ - I will look into that once I get confirmation that those variables and their names are the right way to go.
Reporter

Comment 3

11 years ago
Sorry for the fast patch update here, but I ran into a few obvious bugs while testing on Linux with the actual "pretty" names.

What I have tested and found to be working on Linux now (on a trunk build with --enable-update-packaging and --with-l10n-base set to a path that contains a current copy of de):

make package MOZ_PKG_PRETTYNAMES=1 MOZ_PKG_VERSION="3.1beta1" MOZ_PKG_LONGVERSION="3.1 Beta 1"
make -C tools/update-packaging MOZ_PKG_PRETTYNAMES=1 MOZ_PKG_VERSION="3.1beta1" MOZ_PKG_LONGVERSION="3.1 Beta 1"
make -C browser/locales/ installers-de  MOZ_PKG_PRETTYNAMES=1 MOZ_PKG_VERSION="3.1beta1" MOZ_PKG_LONGVERSION="3.1 Beta 1"
make -C tools/update-packaging MOZ_PKG_PRETTYNAMES=1 MOZ_PKG_VERSION="3.1beta1" MOZ_PKG_LONGVERSION="3.1 Beta 1" AB_CD=de PACKAGE_BASE_DIR=../../dist/l10n-stage

This results in the following files:

dist/langpack/de.xpi
dist/linux-i686/de/minefield-3.1beta1.tar.bz2
dist/linux-i686/en-US/minefield-3.1beta1.tar.bz2
dist/update/linux-i686/de/minefield-3.1beta1.complete.mar
dist/update/linux-i686/en-US/minefield-3.1beta1.complete.mar


Of course, the long version hasn't been used anywhere here, as that's only used on Windows and Mac, testing there would be appreciated a lot!

bsmedberg couldn't point me to the bug about langpacks currently being inside the platform dirs when actually they aren't platform-specific, but he confirmed it is considered a bug and so I fixed that here.

I also incorporated a fix so that we only need one single override var to tell update-packaging about out build being somewhere else than dist/ for L10n builds. Before that, we needed to override DIST which meant also overriding STAGE_DIR and MAR_BIN to point to the places outside l10n-stage again. With the new PACKAGE_BASE_DIR var, this is not needed any more, so doing L10n complete MARs gets easier for everyone.
Attachment #337071 - Attachment is obsolete: true
Attachment #337098 - Flags: review?(ted.mielczarek)
Attachment #337071 - Flags: review?(ted.mielczarek)
Comment on attachment 337098 [details] [diff] [review]
[checked in] add MOZ_PKG_PRETTYNAMES and deal with subdirs, v1.1

Looks good, r=me.
Attachment #337098 - Flags: review?(ted.mielczarek) → review+
Reporter

Comment 5

11 years ago
This is a straight-forward port of the browser/ changes to mail/ and suite/ so we can supports that new style there as well.
Attachment #339115 - Flags: review?(ted.mielczarek)
Attachment #339115 - Flags: review?(ted.mielczarek) → review+
Reporter

Comment 6

11 years ago
main patch pushed as http://hg.mozilla.org/mozilla-central/rev/b375e5cab4dc and mail/suite part as http://hg.mozilla.org/comm-central/rev/417d21c160a9

I'm marking this fixed even though names of partial MARs aren't touched by this. I'm willing to look into them as well, but I couldn't figure out the naming etc. well enough to include them here.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Assignee

Comment 7

11 years ago
I think I've found a bug here on Windows.

I tried doing:
make installer MOZ_PKG_PRETTYNAMES=1 MOZ_PKG_VERSION="3.1 Beta 1"
and got an error because preprocessor.pl is getting passed arguments without quotes:
        for i in packages-static ; do \
          /bin/perl /e/builds/moz2_debug_slave/mozil                              g/preprocessor.pl -DAB_CD=en-US -DPKG_BASENAME=Minef                              ASENAME=Minefield Setup 3.1 Beta 1 -DMOZ_APP_VERSION                              efox -DMOZ_APP_DISPLAYNAME=Minefield -DMOZILLA_VERSI                              NNT5.2\" -DOSARCH=WINNT -D_CRT_SECURE_NO_DEPRECATE=1                              =1 -DWINVER=0x500 -D_WIN32_WINNT=0x500 -D_WIN32_

I've got a potenial patch to fix this running through the try server right now (just testing that I don't break anything obvious - can't do full tests there, of course).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 8

11 years ago
This patch is dead simple - it just quotes all the uses of PKG_BASENAME and PKG_INST_BASENAME that need to be quoted. I skipped usages like:
FOO = blahblah/$PKG_BASENAME/blah
because IIRC spaces don't matter in these.

I'll prepare a comm-central patch once I know this is okay.
Attachment #341936 - Flags: review?(ted.mielczarek)
Attachment #341936 - Flags: review?(ted.mielczarek) → review+
Assignee

Comment 9

11 years ago
Comment on attachment 341936 [details] [diff] [review]
[backed out] follow-up to support win32

e96a238b497b
Attachment #341936 - Attachment description: follow-up to support win32 → [checked in] follow-up to support win32
Assignee

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
nag, CC me on bugs changing the locales makefiles? Thanks. That way I don't end up surprised to fix merge conflicts.
(In reply to comment #10)
> nag, CC me on bugs changing the locales makefiles? Thanks. That way I don't end
> up surprised to fix merge conflicts.

Will do
I think this completely busted windows, at least the quoting piece.

mozilla@WINDOWS-XP-VM /c/workdir/orig-repacks/browser/locales
$ make repackage-win32-installer-fr
make[1]: Entering directory `/c/workdir/orig-repacks/browser/locales'
Verifying fr installer variable usage
Repackaging  into /c/workdir/orig-repacks/dist/install/sea/firefox-3.1b2pre.fr.win32.installer.exe.
/local/bin/make -C ../installer/windows export
make[2]: Entering directory `/c/workdir/orig-repacks/browser/installer/windows'
/c/mozilla-build//python25/python /z/src/central/mozilla-central/config/nsinstall.py -D ../../../dist/branding
cp /z/src/central/mozilla-central/browser/installer/windows/nsis/branding.nsi ../../../dist/branding/branding.nsi
cp /z/src/central/mozilla-central/browser/installer/windows/wizHeader.bmp     ../../../dist/branding/wizHeader.bmp
cp /z/src/central/mozilla-central/browser/installer/windows/wizHeaderRTL.bmp  ../../../dist/branding/wizHeaderRTL.bmp
cp /z/src/central/mozilla-central/browser/installer/windows/wizWatermark.bmp  ../../../dist/branding/wizWatermark.bmp
make[2]: Leaving directory `/c/workdir/orig-repacks/browser/installer/windows'
if test ! -d "/c/workdir/orig-repacks/dist/install/sea/; then \
           /c/mozilla-build//python25/python /z/src/central/mozilla-central/config/nsinstall.py -D "/c/workdir/orig-repacks/dist/install/sea/; \
        fi
/bin/sh: -c: line 3: syntax error near unexpected token `fi'
/bin/sh: -c: line 3: `fi'
make[1]: *** [repackage-win32-installer] Error 2
make[1]: Leaving directory `/c/workdir/orig-repacks/browser/locales'
make: *** [repackage-win32-installer-fr] Error 2


See the unbalanced quote? If I unquote the variable in the rule again, I get passed that point, but corrupt when trying to unpack the installer.

The wget rule seems to be broken, too, looking at http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla-l10n
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backing out.
Reporter

Updated

11 years ago
Attachment #341936 - Attachment description: [checked in] follow-up to support win32 → [backed out] follow-up to support win32
Reporter

Updated

11 years ago
Attachment #337098 - Attachment description: add MOZ_PKG_PRETTYNAMES and deal with subdirs, v1.1 → [checked in] add MOZ_PKG_PRETTYNAMES and deal with subdirs, v1.1
Reporter

Updated

11 years ago
Attachment #339115 - Attachment description: apply browser/ changes to mail/ and suite/ as well → [checked in] apply browser/ changes to mail/ and suite/ as well
Reporter

Comment 14

11 years ago
Would adding the quotes to the affected *_BASENAME vars in package-name.mk work, or do tools get confused with a path descriptor ending up as dist/win32/en-US/"Firefox Setup 3.1b1".exe?
preprocessor.pl is funky about quotes in itself, maybe it would help to convert over to PreProcessor.py there.

We should in general only quote variables when we pass them to external commands, and not as makefile variables, IMHO. That's just a hack that goes bad easily. For example one reason was that $(dir "/some/path/to/file.ext") returns "/some/path/to. With just one quote. Not sure what $(dir ) does with filenames with spaces.

One quote should probably go to http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/Makefile.in#69, for example. I expect those to be just "", not \"\". I expect the latter to make it through to the preprocessor individual args.
I learned the hard way that Makefiles don't support quoting. It seems there isn't really any nice way to make them deal with spaces in filenames. The best I could find is this s/ /?/ trick. If there's a better way, I'm happy to use it.

Most of this patch is s/ /?/ and quoting (inside of targets, where it actually works).

The other notable changes are in package-name.mk. I had to special case Windows separately from Mac - because we want the installer in "pretty" format but not the zip file. I've hardcoded MOZ_APP_VERSION in all of the "pretty" version filenames. It looks like package-name.mk was originally designed to let you override the version number in the package name - by using MOZ_APP_VERSION I've removed that option when in "pretty" mode. Since releases are the only consumers of this code I think that's OK though.

I'm 99% sure this code works as intended. I've tested it on all 3 platforms with en-US and a locale. I'm still no Makefile expert though, and am open to suggestions (that don't extend beyond the scope of this bug).
Attachment #341936 - Attachment is obsolete: true
Attachment #344041 - Flags: review?(ted.mielczarek)
Assignee

Updated

11 years ago
Attachment #344041 - Flags: review?(l10n)
Comment on attachment 344041 [details] [diff] [review]
try #2 - tested on all three platforms, en-US + de

>diff -r adf33dbb59d0 browser/installer/windows/Makefile.in
<...>
> DEFINES += \
> 	-DAB_CD=$(AB_CD) \
>-	-DPKG_BASENAME=$(PKG_BASENAME) \
>-	-DPKG_INST_BASENAME=$(PKG_INST_BASENAME) \
>+	-DPKG_BASENAME=$(subst, $(space),?,$(PKG_BASENAME)) \
>+	-DPKG_INST_BASENAME=$(subst, $(space),?,$(PKG_INST_BASENAME)) \

This looks odd. Are these used anywhere? I'd expect quotes to work here, and I don't see any corresponding unescaping in the patch, thus I wonder if these defines are actually used.

>diff -r adf33dbb59d0 browser/locales/Makefile.in
<...>
> DEFINES += \
> 	-DAB_CD=$(AB_CD) \
> 	-DMOZ_LANGPACK_EID=langpack-$(AB_CD)@firefox.mozilla.org \
> 	-DMOZ_APP_VERSION=$(MOZ_APP_VERSION) \
> 	-DLOCALE_SRCDIR=$(call core_abspath,$(LOCALE_SRCDIR)) \
>-	-DPKG_BASENAME=$(PKG_BASENAME) \
>-	-DPKG_INST_BASENAME=$(PKG_INST_BASENAME) \
>+	-DPKG_BASENAME=$(subst $(space),?,$(PKG_BASENAME)) \
>+	-DPKG_INST_BASENAME=$(subst $(space),?,$(PKG_INST_BASENAME)) \
> 	$(NULL)

Same here.

<...>

>-repackage-win32-installer: WIN32_INSTALLER_OUT=$(_ABS_DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME).exe
>+repackage-win32-installer: WIN32_INSTALLER_OUT=$(_ABS_DIST)/$(PKG_INST_PATH)$(subst $(space),?,$(PKG_INST_BASENAME)).exe
> repackage-win32-installer: $(WIN32_INSTALLER_IN) $(SUBMAKEFILES)
<...>
>-	    app.7z > $(WIN32_INSTALLER_OUT)
>+	    app.7z > "$(subst ?,$(space),$(WIN32_INSTALLER_OUT))"
> 	chmod 0755 $(WIN32_INSTALLER_OUT)

What happens if you don't escape above, and just quote both the redirect, and the chmod. The chmod works because the ? are expanded as wildcards?

The rest of the patch doesn't pose any questions on my side, leaving the review open for more information.
(In reply to comment #17)
> (From update of attachment 344041 [details] [diff] [review])
> >diff -r adf33dbb59d0 browser/installer/windows/Makefile.in
> <...>
> > DEFINES += \
> > 	-DAB_CD=$(AB_CD) \
> >-	-DPKG_BASENAME=$(PKG_BASENAME) \
> >-	-DPKG_INST_BASENAME=$(PKG_INST_BASENAME) \
> >+	-DPKG_BASENAME=$(subst, $(space),?,$(PKG_BASENAME)) \
> >+	-DPKG_INST_BASENAME=$(subst, $(space),?,$(PKG_INST_BASENAME)) \
> 
> This looks odd. Are these used anywhere? I'd expect quotes to work here, and I
> don't see any corresponding unescaping in the patch, thus I wonder if these
> defines are actually used.
> 

These all get passed to preprocessor.pl - which will bail with spaces. I'm pretty sure this didn't work with quotes, but I'll test it again.

> >-repackage-win32-installer: WIN32_INSTALLER_OUT=$(_ABS_DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME).exe
> >+repackage-win32-installer: WIN32_INSTALLER_OUT=$(_ABS_DIST)/$(PKG_INST_PATH)$(subst $(space),?,$(PKG_INST_BASENAME)).exe
> > repackage-win32-installer: $(WIN32_INSTALLER_IN) $(SUBMAKEFILES)
> <...>
> >-	    app.7z > $(WIN32_INSTALLER_OUT)
> >+	    app.7z > "$(subst ?,$(space),$(WIN32_INSTALLER_OUT))"
> > 	chmod 0755 $(WIN32_INSTALLER_OUT)
> 
> What happens if you don't escape above, and just quote both the redirect, and
> the chmod. The chmod works because the ? are expanded as wildcards?
>

I have to use $(subst) at the top of this section - quotes absolutely don't work there. Thus, I have to unquote it for the redirect, otherwise I got a "no such file or a directory" error.

The chmod is fine with the wildcards because they are expanded, as you said.
preprocessor.pl may need something as funky as 

-DPKG_BASENAME="\"$(PKG_BASENAME)\""

but seriously, if subst-ing the spaces doesn't break anything, you should be able to remove those defines alltogether.

Regarding the second, I was thinking about

repackage-win32-installer: WIN32_INSTALLER_OUT=$(_ABS_DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME).exe
...
	app.7z > "$(WIN32_INSTALLER_OUT)"
	chmod 0755 "$(WIN32_INSTALLER_OUT)"

I.e., no quotes in the rule definition, but only in the shell commands.
Comment on attachment 344041 [details] [diff] [review]
try #2 - tested on all three platforms, en-US + de

So, it looks like my checkouts were before bug 458014 landed, and thus I didn't test against the right code. Updated patch to come later.
Attachment #344041 - Flags: review?(ted.mielczarek)
Attachment #344041 - Flags: review?(l10n)
Reporter

Comment 21

11 years ago
What are those changes in package-name.mk about? they effectively change what filenames we generate and make us generate something different than what we did before. They also introduce capital letter in unix names which is something we never did.
Reporter

Comment 22

11 years ago
Actually, the changes to package-name.mk completely are undoing some very much intended possibilities of overriding variables when calling this, IMHO this is unacceptable.
Posted patch pretty names, en-US only (obsolete) — Splinter Review
Okay, Axel's latest locales/Makefile.in patch has complicated matters there so this patch only addresses en-US.

The interesting stuff is in package-name.mk. I've given that file the ability to generate the pretty names now - this eliminates the need to pass in MOZ_PKG_VERSION (but, it is still possible to, if you wanted to override the version number).

Kairo, I'm asking ted to review this but I'd still like to know if this addresses your concerns.
Attachment #344041 - Attachment is obsolete: true
Attachment #344499 - Flags: review?(ted.mielczarek)
Reporter

Comment 24

11 years ago
Comment on attachment 344499 [details] [diff] [review]
pretty names, en-US only

>diff -r f4cecdaa6e78 toolkit/mozapps/installer/package-name.mk
>--- a/toolkit/mozapps/installer/package-name.mk	Thu Oct 23 08:30:45 2008 -0400
>+++ b/toolkit/mozapps/installer/package-name.mk	Thu Oct 23 13:51:51 2008 -0400
>@@ -105,26 +105,35 @@
> 
> else # "pretty" release package names
> 
> ifndef MOZ_PKG_APPNAME
> MOZ_PKG_APPNAME = $(MOZ_APP_DISPLAYNAME)
> endif
> MOZ_PKG_APPNAME_LC = $(shell echo $(MOZ_PKG_APPNAME) | tr '[A-Z]' '[a-z]')
> 
>+
> ifndef MOZ_PKG_LONGVERSION

nit: why two empty line if one should be enough?

>-MOZ_PKG_LONGVERSION = $(MOZ_PKG_VERSION)
>+MOZ_PKG_LONGVERSION = $(shell echo $(MOZ_PKG_VERSION) |\
>+                       sed -e 's/a\([0-9][0-9]*\)/ Alpha \1/' |\
>+                       sed -e 's/b\([0-9][0-9]*\)/ Beta \1/' |\
>+                       sed -e 's/rc\([0-9][0-9]*\)/ RC \1/')
> endif

nice :)
I still wonder how you generate "shiretoko-alpha1.tar.bz2" etc., I guess you still need to pass both different version numbers there, there's probably no way around that.

>-ifeq (,$(filter-out Darwin OS2 WINNT, $(OS_ARCH)))
>+ifeq (,$(filter-out Darwin OS2, $(OS_ARCH))) # Mac and OS2
> PKG_BASENAME = $(MOZ_PKG_APPNAME) $(MOZ_PKG_LONGVERSION)
>+PKG_INST_BASENAME = $(MOZ_PKG_APPNAME) Setup $(MOZ_PKG_LONGVERSION)
>+else
>+ifeq (,$(filter-out WINNT, $(OS_ARCH))) # Windows
>+PKG_BASENAME = $(MOZ_PKG_APPNAME_LC)-$(MOZ_PKG_VERSION)
> PKG_INST_BASENAME = $(MOZ_PKG_APPNAME) Setup $(MOZ_PKG_LONGVERSION)
> else # unix (actually, not Windows, Mac or OS/2)
> PKG_BASENAME = $(MOZ_PKG_APPNAME_LC)-$(MOZ_PKG_VERSION)
> PKG_INST_BASENAME = $(MOZ_PKG_APPNAME_LC)-setup-$(MOZ_PKG_VERSION)
>+endif
> endif

Not sure about the different treatment of Windows zip and installer here, is there any specific reason for that? And it sounds a bit strange that OS/2 and Windows are treated differently ;-)
Whoops, the extra space was unintentional. You're absolutely right that this doesn't handle Alphas. We'll cross that bridge when we get to it. (We may fix that be making alpha dir structure and filename the same as beta, dunno.)

As for the zip files...that's just how we distribute them. I'm not seeking to change that right now. The actual reason is probably more historical than logical, dunno!
Reporter

Comment 26

11 years ago
(In reply to comment #25)
> As for the zip files...that's just how we distribute them. I'm not seeking to
> change that right now. The actual reason is probably more historical than
> logical, dunno!

Erm, it's news to me that Firefox or Thunderbird even distribute zip files in releases - and nightlies as well as SeaMonkey releases don't use pretty names ;-)
That's a good point. We do need the en-US zip file to do repacks (I really don't know why, that's just how the system works).

Maybe we can drop them alltogether at some point? hopefully.
What's actually breaking for non-en-US?

Regarding the zips on Windows, that's currently the easiest format to unpack, and I use that to identify the source stamp of the repackaged build. That step is probably not needed for release builds, as those build off tags, and thus can get their known sources anyway.
Reporter

Comment 29

11 years ago
I think it's good that we are actually packaging zips, I know people who regularly use them for nightlies and for SeaMonkey, so I'd rather not get rid of them. But if we aren't actually shipping them for releases, I don't think we need to special-case them in package-name.mk and can merge the Mac, OS/2 and Windows cases on the prettyname side again ;-)
We do use them for releases, though. They are used in l10n repacks. As I mentioned in comment#25 I'm not looking to change that as part of this bug. Once we have working infrastructure for l10n + releases on Mercurial we can look at that.

(It kindof sucks that package-name.mk is shared across all projects.)
(In reply to comment #28)
> What's actually breaking for non-en-US?
> 

We talked about this on IRC, remember? :) bug 461340 - browser/locales/Makefile.in broken when not using wget-* targets.
Comment on attachment 344499 [details] [diff] [review]
pretty names, en-US only

+MOZ_PKG_LONGVERSION = $(shell echo $(MOZ_PKG_VERSION) |\
+                       sed -e 's/a\([0-9][0-9]*\)/ Alpha \1/' |\
+                       sed -e 's/b\([0-9][0-9]*\)/ Beta \1/' |\
+                       sed -e 's/rc\([0-9][0-9]*\)/ RC \1/')

Could you make these regexps end with '$'? (I know the quoting will probably get ugly in the makefile.)
Attachment #344499 - Flags: review?(ted.mielczarek) → review+
same as before, with the terminating $s

changeset:   20812:1a342e4331ff
Assignee: kairo → bhearsum
Attachment #344499 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Assignee

Updated

11 years ago
Whiteboard: [hg-automation]
Assignee

Updated

11 years ago
Duplicate of this bug: 464156
Assignee

Updated

11 years ago
Blocks: 464154
Going to try and get this working for locales now...
Priority: -- → P2
Alright, so this was less troublesome than I thought it would be. Some of this patch is just quoting where necessary, but there are some other changes:
* Use ZIP_IN for repackage-zip stuff (and set ZIP_IN to default to $(_ABS_DIST)/$(PACKAGE) as to not break existing code that doesn't pass it)
* Don't use $(dir) when testing for existence in repackage-win32-installer-%. This is necessary because $(dir) does not work with spaces in the filename.
* When using MOZ_PKG_PRETTYNAMES we have to move the unpacked package from a directory named after $(MOZ_APP_NAME) to one named after $(MOZ_PKG_APPNAME). If we don't do tools/update-packaging won't be able to find the directory to create a MAR from. Mac does this in it's $(UNMAKE_PACKAGE) anyways, so we only do this for Linux and Windows.

I also discovered that the regexps in package-name.mk are broken, so I fixed them.
Attachment #348007 - Flags: review?(ted.mielczarek)
Attachment #348007 - Flags: review?(l10n)
Comment on attachment 348007 [details] [diff] [review]
MOZ_PKG_PRETTYNAMES support for l10n

wrong patch, will reattach
Attachment #348007 - Attachment is obsolete: true
Attachment #348007 - Flags: review?(ted.mielczarek)
Attachment #348007 - Flags: review?(l10n)
See my previous comment for details.
Attachment #348008 - Flags: review?(ted.mielczarek)
Attachment #348008 - Flags: review?(l10n)
Comment on attachment 348008 [details] [diff] [review]
MOZ_PKG_PRETTYNAMES support for l10n

+ifndef ZIP_IN
+ZIP_IN = $(_ABS_DIST)/$(PACKAGE)
+endif

You can write this as
ZIP_IN ?= $(_ABS_DIST)/$(PACKAGE)
Attachment #348008 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 348008 [details] [diff] [review]
MOZ_PKG_PRETTYNAMES support for l10n

r=me, with ted's nit.

As per IRC, MOZ_PKG_PRETTYNAMES only works when ZIP_IN is explicitly set to something non-pretty.

Add a comment to that extent, too?
Attachment #348008 - Flags: review?(l10n) → review+
Carrying review forward here.
Attachment #348008 - Attachment is obsolete: true
Comment on attachment 348192 [details] [diff] [review]
[checked in] MOZ_PKG_PRETTYNAMES for l10n - comments addressed

Mike, this is NPOT en-US Build and would really help me with 3.1b2 l10n builds. Any chance I can get this in for 3.1b2?
Attachment #348192 - Flags: approval1.9.1b2?
Assignee

Updated

11 years ago
Attachment #348192 - Flags: approval1.9.1b2?
Comment on attachment 348192 [details] [diff] [review]
[checked in] MOZ_PKG_PRETTYNAMES for l10n - comments addressed

Mossop landed this for me in b7a7180130d5.

I'll be watching Mozilla-l10n closely for breakage.
Attachment #348192 - Attachment description: MOZ_PKG_PRETTYNAMES for l10n - comments addressed → [checked in] MOZ_PKG_PRETTYNAMES for l10n - comments addressed
Assignee

Updated

11 years ago
Duplicate of this bug: 461340
I just saw a run of 'uk' go by fine on bsmedberg's l10n buildbot. I'll be watching the l10n Buildbot on staging-master too, but I'm pretty confident everything will be OK.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
So, it turns out that all my testing was done by repacking nightly builds. The big difference here is that nightly en-US builds are packaged without MOZ_PKG_PRETTYNAMES, which means the containing directories are 'firefox'. Release builds are (now) packaged with MOZ_PKG_PRETTYNAMES, which means the containing directory is 'Firefox'. What we should probably do here is removing the 'mv firefox Firefox' that's done in STAGE_DIST and simply declare that if the en-US was packaged with MOZ_PKG_PRETTYNAMES that you must package the l10n builds that way as well.

I'll be looking into this more after 3.1b2.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This problem bit me again in our update testing scripts. It also caused some confusion to folks who downloaded the Linux tarball. I also wonder if there are Linux packagers who take our tarball (gentoo?) - this could break them as well. I'm not going back to fix it for 3.1b2 at this point but going forward I think we should revert back to 'firefox'. This seems to be an unintended change, anyways.
Reporter

Comment 48

11 years ago
Yes, this is unintended, it shouldn't be different than before... did we have this named "shiretoko" or "firefox" for e.g. alpha 1 (or for BonEcho alphas)?
Just to be clear, I'm talking about the first level directory inside of a tarball or zip. Other than for this build I don't know of any build of Firefox which has named this directory differently. Eg:
bitters-2:tmp bhearsum$ tar -jvxf shiretoko-alpha2.tar.bz2 
firefox/
Reporter

Comment 50

11 years ago
Here's a patch for the in-package (and in-dist) directory to not end up with the display name used for the package name even with "pretty" names. For this, I've created another var in packager.mk named MOZ_PKG_DIR because I felt the code would be harder to understand if I'd place MOZ_APPNAME there, as a side effect, it can be overridden - for whatever reason someone might want that.
I've only tested this on Linux, where a prettynames package that calls itself "minefield-something.tar.bz2" correctly ended up with a firefox/ directory inside.
Attachment #350399 - Flags: review?(ted.mielczarek)
Given that we're fixing the directories inside of the packages to remain lowercase we don't need this special case anymore.
Attachment #351550 - Flags: review?(ted.mielczarek)
Attachment #351550 - Flags: review?(l10n)
Attachment #350399 - Flags: review?(ted.mielczarek) → review+
Attachment #351550 - Flags: review?(ted.mielczarek) → review+

Updated

11 years ago
Attachment #351550 - Flags: review?(l10n)
Comment on attachment 351550 [details] [diff] [review]
[checked in] remove MOZ_PKG_PRETTYNAMES special case in Makefile.in

I don't really understand how this interacts with the universe, so I'll go with Ted's r, and I'm canceling this review on me.
Assignee

Updated

11 years ago
Attachment #350399 - Attachment description: create a MOZ_PKG_NAME for the in-package dir → [checked in] create a MOZ_PKG_NAME for the in-package dir
Comment on attachment 350399 [details] [diff] [review]
[checked in] create a MOZ_PKG_NAME for the in-package dir

o  changeset:   22375:53326bcc0158
Attachment #351550 - Attachment description: remove MOZ_PKG_PRETTYNAMES special case in Makefile.in → [checked in] remove MOZ_PKG_PRETTYNAMES special case in Makefile.in
Comment on attachment 351550 [details] [diff] [review]
[checked in] remove MOZ_PKG_PRETTYNAMES special case in Makefile.in

@  changeset:   22376:5b62f3c644cb
I'm pretty sure this is done now (finally)
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Ugh, I forgot that I still need to land these on mozilla-1.9.1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

11 years ago
Attachment #350399 - Flags: approval1.9.1?
Comment on attachment 350399 [details] [diff] [review]
[checked in] create a MOZ_PKG_NAME for the in-package dir

This patch is required for release automation going forward. It gently touches the packaging code on all platforms but has cycled green on mozilla-central already. Given that, I feel it is very low risk.
Attachment #350399 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 351550 [details] [diff] [review]
[checked in] remove MOZ_PKG_PRETTYNAMES special case in Makefile.in

changeset:   22217:89bf1ad2d30f (mozilla-1.9.1)
Assignee

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b3
Assignee

Updated

11 years ago
Target Milestone: mozilla1.9.1b3 → ---
Comment on attachment 351550 [details] [diff] [review]
[checked in] remove MOZ_PKG_PRETTYNAMES special case in Makefile.in

Turns out I forgot to land this in mozilla-1.9.1. Landed now in:
changeset:   22788:a28d67a980fe
Reporter

Updated

11 years ago
Blocks: 475120

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.