Closed Bug 741014 Opened 11 years ago Closed 11 years ago

pymake is busted with make l10n-check


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: catlee, Assigned: rain1)




(2 files, 1 obsolete file)

see bug 593585#c30 and below
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
Oh, nvm, I can reproduce with MOZ_PKG_PRETTYNAMES=1.
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.
The three requirements being "Firefox\", "Setup\" and "17.0a1.exe" all without quotes.
This probably isn't the exact issue we're seeing, but it makes pymake crash immediately so it's a little more useful :)
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 <>.
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.)
Yeah, I think it's assumed that the file should already exists.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #648196 - Flags: review?(ted.mielczarek)
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).
Attachment #648196 - Flags: review?(khuey)
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.
Yeah, shell has somewhat different semantics. What a mess.
This patch might be wrong. I'll investigate tomorrow.
Attached patch better patchSplinter Review
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).
Attachment #648196 - Attachment is obsolete: true
Attachment #648196 - Flags: review?(ted.mielczarek)
Attachment #648196 - Flags: review?(khuey)
Attachment #648577 - Flags: review?(ted.mielczarek)
Attachment #648577 - Flags: review?(khuey)
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?
Attachment #648577 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → sagarwal
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.