Note: There are a few cases of duplicates in user autocompletion which are being worked on.

pymake is busted with make l10n-check

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: catlee, Assigned: sid0)

Tracking

Trunk
mozilla17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
see bug 593585#c30 and below
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
Oh, nvm, I can reproduce with MOZ_PKG_PRETTYNAMES=1.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
The three requirements being "Firefox\", "Setup\" and "17.0a1.exe" all without quotes.
(Assignee)

Comment 5

5 years ago
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 :)
(Assignee)

Comment 6

5 years ago
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>.
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.)
(Assignee)

Comment 8

5 years ago
Yeah, I think it's assumed that the file should already exists.
(Assignee)

Comment 9

5 years ago
s/should//
Created attachment 648196 [details] [diff] [review]
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.
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 upload.py. 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.
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).
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/packager.mk
@@ +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+
Attachment #648577 - Flags: review?(khuey)
http://hg.mozilla.org/integration/mozilla-inbound/rev/d001f0761391
Assignee: nobody → sagarwal
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/d001f0761391
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.