741014 - pymake is busted with make l10n-check

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris AtLee [:catlee]]

see bug 593585#c30 and below

Comment 1

[Rain [:rain1] (inactive)]

I can't reproduce the issue in comment 30 locally.

Instead, I get a different issue, ending with

sed: can't read components/*.manifest: No such file or directory
c:\Users\Sid\mozilla\mozilla-central\toolkit\locales\l10n.mk:147:0: command '(cd c:/Users/Sid/mozilla/fxto/browser/locales/../../dist/l10n-stage/firefox && c:/Users/Sid/mozilla/fxto/_virtualenv/Scripts/python.exe c:/Users/Sid/mozilla/fxto/browser/locales/../../../mozilla-central/config/optimizejars.py --deoptimize c:/Users/Sid/mozilla/fxto/browser/locales/../../jarlog//x-test ./ ./ && unzip -o omni.ja && rm -f components/binary.manifest && for m in components/*.manifest; do sed -e 's/^#binary-component/binary-component/' $m > tmp.manifest && mv tmp.manifest $m; done)' failed, return code 2

Comment 2

[Rain [:rain1] (inactive)]

Oh, nvm, I can reproduce with MOZ_PKG_PRETTYNAMES=1.

Comment 3

[Rain [:rain1] (inactive)]

The problem is that $(WIN32_INSTALLER_IN) at https://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#159 is "Firefox Setup 17.0a1.exe" without quotes (for example), which is escaped by ESCAPE_SPACE to "Firefox\ Setup\ 17.0a1.exe" without quotes. Pymake parses this not as one requirement but as three.

Comment 4

[Rain [:rain1] (inactive)]

The three requirements being "Firefox\", "Setup\" and "17.0a1.exe" all without quotes.

Comment 5

[Rain [:rain1] (inactive)]

Attachment: test demonstrating a related issue

This probably isn't the exact issue we're seeing, but it makes pymake crash immediately so it's a little more useful :)

Comment 6

[Rain [:rain1] (inactive)]

Of course, a reasonable argument could be made that GNU make was never meant to support spaces, so we shouldn't even bother and should instead rename the setup file. See <http://savannah.gnu.org/bugs/?712>.

Comment 7

[(not currently active) Ted Mielczarek]

We work around this with QUOTED_WILDCARD elsewhere, not sure if that will work in this case:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#973

(It only really works if the file already exists.)

Comment 8

[Rain [:rain1] (inactive)]

Yeah, I think it's assumed that the file should already exists.

Comment 9

[Rain [:rain1] (inactive)]

s/should//

Comment 10

[Rain [:rain1] (inactive)]

Attachment: patch v1

So... I've switched to escaping via wildcards (for a comparison, see http://www.cmcrossroads.com/ask-mr-make/7859-gnu-make-meets-file-names-with-spaces-in-them). This is different from QUOTED_WILDCARD -- neither GNU make nor pymake recognizes escaping via quotes.

There was also an underspecified dependency: repackage-win32-installer-% depends on repackage-zip-% because the repackage-zip step is the one that unzips the data needed to create the repackaged installer. It was pure chance that was making it work earlier.

With these two problems fixed, pymake l10n-check seems to work great.

Comment 11

[Rain [:rain1] (inactive)]

Comment on attachment 648196 [details] [diff] [review]
patch v1

Given how hairy and fragile this code is, I'd like both khuey and ted to look at it (unless one of you feels your review is enough).

Comment 12

[(not currently active) Ted Mielczarek]

I believe the quoting part of QUOTED_WILDCARD is just to make it work properly when we pass it on the commandline to upload.py. The escaping with wildcard characters is to make $(wildcard) happy.

Comment 13

[Rain [:rain1] (inactive)]

Yeah, shell has somewhat different semantics. What a mess.

Comment 14

[Rain [:rain1] (inactive)]

This patch might be wrong. I'll investigate tomorrow.

Comment 15

[Rain [:rain1] (inactive)]

Attachment: better patch

The dependency is actually the other way around (repackage-zip depends on repackage-win32-installer files generated in the l10ngen directory), though both depend on $(STAGEDIST).

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 648577 [details] [diff] [review]
better patch

Review of attachment 648577 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/installer/packager.mk
@@ +976,2 @@
>  
>  # This variable defines which OpenSSL algorithm to use to 

Kill the trailing space on this line while you're here?

Comment 17

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d001f0761391

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d001f0761391