Last Comment Bug 741014 - pymake is busted with make l10n-check
: pymake is busted with make l10n-check
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Siddharth Agarwal [:sid0] (inactive)
: Gregory Szorc [:gps]
Depends on:
Blocks: 593585
  Show dependency treegraph
Reported: 2012-03-30 16:45 PDT by Chris AtLee [:catlee]
Modified: 2012-08-08 09:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

test demonstrating a related issue (226 bytes, patch)
2012-08-01 14:09 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
patch v1 (3.56 KB, patch)
2012-08-01 19:59 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
better patch (3.65 KB, patch)
2012-08-02 17:14 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Splinter Review

Description Chris AtLee [:catlee] 2012-03-30 16:45:27 PDT
see bug 593585#c30 and below
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 13:15:21 PDT
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\ 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/ --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 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 13:30:36 PDT
Oh, nvm, I can reproduce with MOZ_PKG_PRETTYNAMES=1.
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 13:42:47 PDT
The problem is that $(WIN32_INSTALLER_IN) at 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 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 13:44:46 PDT
The three requirements being "Firefox\", "Setup\" and "17.0a1.exe" all without quotes.
Comment 5 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 14:09:34 PDT
Created attachment 648075 [details] [diff] [review]
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 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 14:16:27 PDT
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 <>.
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-08-01 16:35:41 PDT
We work around this with QUOTED_WILDCARD elsewhere, not sure if that will work in this case:

(It only really works if the file already exists.)
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 16:38:28 PDT
Yeah, I think it's assumed that the file should already exists.
Comment 9 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 16:39:14 PDT
Comment 10 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 19:59:35 PDT
Created attachment 648196 [details] [diff] [review]
patch v1

So... I've switched to escaping via wildcards (for a comparison, see 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 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 20:01:20 PDT
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 Ted Mielczarek [:ted.mielczarek] 2012-08-02 05:59:58 PDT
I believe the quoting part of QUOTED_WILDCARD is just to make it work properly when we pass it on the commandline to The escaping with wildcard characters is to make $(wildcard) happy.
Comment 13 Siddharth Agarwal [:sid0] (inactive) 2012-08-02 06:53:46 PDT
Yeah, shell has somewhat different semantics. What a mess.
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2012-08-02 16:37:05 PDT
This patch might be wrong. I'll investigate tomorrow.
Comment 15 Siddharth Agarwal [:sid0] (inactive) 2012-08-02 17:14:47 PDT
Created attachment 648577 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2012-08-06 10:42:14 PDT
Comment on attachment 648577 [details] [diff] [review]
better patch

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

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

Kill the trailing space on this line while you're here?
Comment 17 Siddharth Agarwal [:sid0] (inactive) 2012-08-07 12:23:05 PDT
Comment 18 Ed Morley [:emorley] 2012-08-08 09:34:20 PDT

Note You need to log in before you can comment on or make changes to this bug.