Closed Bug 525438 Opened 10 years ago Closed 9 years ago

l10n-merge doesn't merge all files required to build

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(blocking2.0 -)

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: stas, Assigned: Pike)

References

Details

(Whiteboard: fixed-in-bs)

Attachments

(9 files, 1 obsolete file)

When doing:

make merge-ab-CD LOCALE_MERGEDIR=mergedir
make (langpack|installers)-ab-CD LOCALE_MERGEDIR=mergedir

the following files need to exist in the locale's directory:

browser/README.txt
browser/profile/chrome/userChrome-example.css
browser/profile/chrome/userContent-example.css
browser/searchplugins/list.txt
browser/updater/updater.ini
toolkit/defines.inc


There are 3 types of errors that I see ("x-testing" is the locale code that I'm using for tests):


1. "needed by 'libs'"

make[1]: *** No rule to make target `/Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/browser/README.txt', needed by `libs'.  Stop.
make[1]: *** No rule to make target `/Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/browser/profile/chrome/userChrome-example.css', needed by `libs'.  Stop.
make[1]: *** No rule to make target `/Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/browser/profile/chrome/userContent-example.css', needed by `libs'.  Stop.
make[1]: *** No rule to make target `/Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/browser/updater/updater.ini', needed by `libs'.  Stop.


2. nsinstall.py expecting more args (for searchplugins/list.txt)

/usr/bin/python2.5 /Users/stas/Mozilla/vc/koala-l10n/application/Firefox/3.6/config/nsinstall.py -t -m 644  ../../dist/xpi-stage/locale-x-testing/searchplugins
Usage: nsinstall.py [options] arg1 [arg2 ...] target-directory

nsinstall.py: error: not enough arguments
make[1]: *** [libs] Error 2


3. passing locale's toolkit/defines.inc to JarMaker.py without a fall-back to en-US.

/usr/bin/python2.5 /Users/stas/Mozilla/vc/koala-l10n/application/Firefox/3.6/config/JarMaker.py \
	   -j ../../dist/xpi-stage/locale-x-testing/chrome \
	  -t /Users/stas/Mozilla/vc/koala-l10n/application/Firefox/3.6 -f jar  --both-manifests -c mergedir/toolkit -c /Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/toolkit -c /Users/stas/Mozilla/vc/koala-l10n/application/Firefox/3.6/toolkit/locales/en-US -I/Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/toolkit/defines.inc -DNDEBUG (…lots of other flags…) /Users/stas/Mozilla/vc/koala-l10n/application/Firefox/3.6/toolkit/locales/jar.mn

Preprocessor.Error: ('', 0, 'FILE_NOT_FOUND', '/Users/stas/Mozilla/vc/koala-l10n/locales/3.6/x-testing/toolkit/defines.inc')
I'm having a local patch that I'm pounding right now.

Target is to get down to toolkit/defines.inc, which I keep to have a language in the UI to switch to.

Making this block bug 588746, as we'll have an easier time to do vanilla repack test builds if it's just one defined file that's allowed to miss.
Assignee: nobody → l10n
Blocks: 588746
Here's what I'm doing:

I'm adding two rules/macros to config/config.mk (and in js, sob) to find an l10n-merged file, or run the first macro on a series of files, once for each. So I can get one file from mergedir, one from l10n, one from en-US, all in a list. That's the first patch. Plus tests.

The test creates a dummy locale x-test with just the toolkit/defines.inc and repacks that. The test itself depends on having run make package before. It's basically a check:: target, just that I don't think that everyone wants to run that at home, also the requirement of make package sounded awkward to me. So I created l10n-check:: instead, and added it to builds.mk so that it's a top-level target. The intention is to get that target run as part of regular firefox builds, both try and not. For developers in the packaging area, you could just run make l10n-checks explicitly.

That's great for all but windows, which needs installer magic dragon foo. For that, I added l10n-merge sparkle dust to preprocess-locale.py, in a backwards compatible manner. As I was adding options, the second patch reworks that script to use optparse. On top of that, there's another patch that actually adds the capabilities to pass multiple locale dirs, and hooks that up in the Makefile.in, at least for browser.

Running a patch queue trough try, http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry&rev=4cfb3752fab4.

Windows passed this time around, so I'll attach patches for feedback.

The neatest thing about this is that I can actually just land a three lines patch on top and try-server already does a repack attempt for me already.

The biggest open question I'd have is, what kind of testing would we want to do on in addition of just make installers-x-test?
Attachment #481666 - Flags: review?(ted.mielczarek)
Attachment #481667 - Flags: review?(ted.mielczarek)
Attachment #481668 - Flags: review?(ted.mielczarek)
The last patch is really just an FYI and how I made the try server run the previous code without explicit action. Build-wise, it'd be much more constructive to have this as separate step, so there's no intention on my side to actually land this for realz.
I'm giving up on the PRETTYNAMES, I'll make the test just be a noop if MOZ_PKG_PRETTYNAMES is defined.
Updated patch 1 to disable the test if we have MOZ_PKG_PRETTYNAMES set. The release factories do all kind of quirks in that case to actually not use the PRETTYNAMES for the repack, and thus testing that path without the quirks gives little value anyway.
Attachment #481666 - Attachment is obsolete: true
Attachment #481666 - Flags: review?(ted.mielczarek)
Attachment #481667 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 481668 [details] [diff] [review]
[patch 3] add multiple locale dirs and use for l10n-merge

># HG changeset patch
># Parent 1726d06749b3a31dec7f25827184447acb298b00
>bug 525438, add l10n-merge to preprocess-locale.py
>
>diff --git a/browser/installer/windows/Makefile.in b/browser/installer/windows/Makefile.in
>--- a/browser/installer/windows/Makefile.in
>+++ b/browser/installer/windows/Makefile.in
>@@ -77,6 +77,15 @@
> 
> include $(topsrcdir)/config/config.mk
> 
>+ifeq (,$(LOCALE_MERGEDIR))

Does ifndef LOCALE_MERGEDIR not work here?

>+PPL_LOCALE_ARGS=$(call EXPAND_LOCALE_SRCDIR,browser/locales)/installer
>+else
>+PPL_LOCALE_ARGS = --l10n-dir=$(LOCALE_MERGEDIR)/browser/installer \

Nits: put the first argument on its own line here, please. Also, we prefer two-space indent after continuations.
Attachment #481668 - Flags: review?(ted.mielczarek) → review+
Ted, not sure if that helps or is worse, but I figured this might be easier to review. This is a full patch queue in one hg out -p. The queue works like this:

0 A merge-more.patch
1 A escape-package.patch
2 A preproc-optparse.patch
3 A preproc-merge.patch

merge-more is doing more merges, and only changes in the way that it's carrying over your review nit about multi-line defines.

escape-package.patch is new and makes MOZ_PKG_PRETTYNAMES work.

preproc-optparse is unchanged, preproc-merge got nits, including the multi-line stuff.

Tested for fx on win, mac, linux, with and without prettnames. Win tested with ZIP_IN and WIN32_INSTALLER_IN like the release factory does, with pretty names and complete mar. Tested on fennec linux desktop, too.
Attachment #493770 - Flags: review?(ted.mielczarek)
Comment on attachment 493770 [details] [diff] [review]
complete patch queue

Requesting feedback from bhearsum, shouldn't affect release infra according to my tests.
Attachment #493770 - Flags: feedback?(bhearsum)
Rail, I don't think this should conflict with your work in 313956, but you could see if it helps you getting MOZ_PKG_PRETTYNAMES incorporated.
I really don't have the Makefile-knowledge to do give good feedback on this...I'll try to run it through release scenarios this week, though.
Trying to test this in staging today.
This passed on Linux and Mac using the existing release builders. On Windows, it worked but appeared to do things more than once

I'll upload logs of the installers-% and l10n-upload-% targets in case you're interested. The Windows stuff would really hurt our release repack times, so hopefully a fix can be found.
Attachment #496303 - Attachment is patch: false
(In reply to comment #19)
> This passed on Linux and Mac using the existing release builders. On Windows,
> it worked but appeared to do things more than once
> 
> I'll upload logs of the installers-% and l10n-upload-% targets in case you're
> interested. The Windows stuff would really hurt our release repack times, so
> hopefully a fix can be found.

I just compared the win32.log with a nightly win32 central log, there's no regression in terms of how often it does things, AFAICT.
(In reply to comment #20)
> (In reply to comment #19)
> > This passed on Linux and Mac using the existing release builders. On Windows,
> > it worked but appeared to do things more than once
> > 
> > I'll upload logs of the installers-% and l10n-upload-% targets in case you're
> > interested. The Windows stuff would really hurt our release repack times, so
> > hopefully a fix can be found.
> 
> I just compared the win32.log with a nightly win32 central log, there's no
> regression in terms of how often it does things, AFAICT.

Wow, right you are -- I see the same thing happening in release logs in staging. Only on trunk though, AFAICT. That sucks, but certainly doesn't need to be addressed here. Given that, feedback+ from me.
Attachment #493770 - Flags: feedback?(bhearsum) → feedback+
Comment on attachment 493770 [details] [diff] [review]
complete patch queue

>diff --git a/browser/locales/Makefile.in b/browser/locales/Makefile.in
>--- a/browser/locales/Makefile.in
>+++ b/browser/locales/Makefile.in
>+# test target, depends on make package
>+# try to repack x-test, with just toolkit/defines.inc being there
>+l10n-check::
>+ifndef MOZ_PKG_PRETTYNAMES # standard package names
>+	$(RM) -rf x-test
>+	$(NSINSTALL) -D x-test/toolkit
>+	echo "#define MOZ_LANG_TITLE Just testing" > x-test/toolkit/defines.inc
>+	$(MAKE) installers-x-test L10NBASEDIR="$(PWD)" LOCALE_MERGEDIR="$(PWD)/mergedir"

Do you want to have some logic to try to verify the results of "make installers-x-test" here? Or is this just a sanity check that it doesn't blow up?

>+else
>+	@echo "Repack test disabled for MOZ_PKG_PRETTYNAMES"

So we're not running these tests on release builds? (Or, er, is that fixed in a later patch in this queue? Sort of confusing to review a patch queue as a set of patches in one attachment, actually.)

>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -833,6 +833,16 @@
> endif
> endif
> 
>+ifdef LOCALE_MERGEDIR
>+MERGE_FILE = $(firstword
>+  $(wildcard $(LOCALE_MERGEDIR)/$(subst /locales,,$(relativesrcdir))/$(1)) \
>+  $(wildcard $(LOCALE_SRCDIR)/$(1)) \
>+  $(srcdir)/en-US/$(1) )

I don't think this actually parses as legal make syntax with the newline on the first line of the definition...

These could probably stand a short comment explaining their purpose. (We've historically been pretty bad about that in {config,rules}.mk.)

>diff --git a/browser/locales/Makefile.in b/browser/locales/Makefile.in
>--- a/browser/locales/Makefile.in
>+++ b/browser/locales/Makefile.in
>@@ -189,31 +189,26 @@
> 	@$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1
> 
> 
>-repackage-win32-installer: WIN32_INSTALLER_OUT="$(_ABS_DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME).exe"
>-repackage-win32-installer: $(WIN32_INSTALLER_IN) $(SUBMAKEFILES)
>+repackage-win32-installer: WIN32_INSTALLER_OUT=$(call ESCAPE_SPACE,$(_ABS_DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME).exe)
>+repackage-win32-installer: $(call ESCAPE_SPACE,$(WIN32_INSTALLER_IN)) $(SUBMAKEFILES) libs-$(AB_CD)
> 	@echo "Repackaging $(WIN32_INSTALLER_IN) into $(WIN32_INSTALLER_OUT)."
> 	$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY) export
> 	$(MAKE) -C ../installer/windows CONFIG_DIR=l10ngen l10ngen/setup.exe l10ngen/7zSD.sfx
> 	$(MAKE) repackage-zip \
> 	  AB_CD=$(AB_CD) \
> 	  MOZ_PKG_FORMAT=SFX7Z \
>-	  ZIP_IN=$(WIN32_INSTALLER_IN) \
>+	  ZIP_IN="$(WIN32_INSTALLER_IN)" \
> 	  ZIP_OUT=$(WIN32_INSTALLER_OUT) \

Is there a reason WIN32_INSTALLER_OUT doesn't need to be quoted as well?

FWIW, if you wind up doing something like this in the future, I'd prefer to either review each little patch separately, or to review one megapatch that is the sum of all applied patches, instead of a series of patches in one attachment. Especially with Bugzilla's diff view, it made it a bit more difficult than necessary.

Also, sorry for the review delay.
Attachment #493770 - Flags: review?(ted.mielczarek) → review+
I managed to get the variables consistent.

I didn't make make check run the repack test, and just had an l10n-check target. I can make that run always, but I was a bit scared off because it requires that you make package before. What's your take?

Also, yes, it just tries to repack, and doesn't do anything fancy in terms of checking for generated files or testing the build. It's a good step forward anywho.

I'm wondering, is this a good thing to land on the buildsystem repo?
Landed and stuck to build-system, http://hg.mozilla.org/projects/build-system/rev/fff61d7cc077
Whiteboard: fixed-in-bs
Is this going to land on 1.9.1/1.9.2/2.0?
Drivers, I'd like to land this patch on mozilla-2.0 in order to enable us to enable bug 600838 on that branch, which gets us testing release-style packaging on nightly builds, which reduces the possibility of hitting issues at release time in the packaging code.
blocking2.0: --- → ?
This isn't going to block macaw, but if you still need it when you get it successfully landed on trunk come back and talk to us.
blocking2.0: ? → -
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.