Closed
Bug 943728
Opened 11 years ago
Closed 11 years ago
Use single quotes for shell quoting instead of double quotes
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files, 4 obsolete files)
197.33 KB,
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
5.29 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
The editor/libeditor/html/tests ones would be metter in the corresponding mochitest.ini, but i'm sure the reason they're still there is that mochitest.ini support-files don't support copying to subdirectories.
Attachment #8340148 -
Flags: review?(mshal)
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8340148 [details] [diff] [review]
Convert a couple custom install rules using double quotes to INSTALL_TARGETS
In case you can get here before mshal.
Attachment #8340148 -
Flags: review?(gps)
Assignee | ||
Comment 3•11 years ago
|
||
For whoever comes first.
Try with mozmake and all other platforms:
https://tbpl.mozilla.org/?tree=Try&rev=b30861d8d66e
(this is expected to fix the l10n-check oranges we have on birch as a side effect)
Try with pymake:
https://tbpl.mozilla.org/?tree=Try&rev=81675c13a12a
Try with pristine GNU make 4.x from upstream git (non-msys) (same rev as mozmake, but without the additional patch):
https://tbpl.mozilla.org/?tree=Try&rev=7e04ed2b35c4
(this is expected to work too \o/ (at least it works locally))
Attachment #8340163 -
Flags: review?(mshal)
Attachment #8340163 -
Flags: review?(gps)
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3)
> Try with pristine GNU make 4.x from upstream git (non-msys) (same rev as
> mozmake, but without the additional patch):
> https://tbpl.mozilla.org/?tree=Try&rev=7e04ed2b35c4
> (this is expected to work too \o/ (at least it works locally))
Yeah, well, i forgot i disable webrtc locally :(
Assignee | ||
Comment 5•11 years ago
|
||
Another attempt, I was missing a lot of .mk files.
With mozmake and other platforms:
https://tbpl.mozilla.org/?tree=Try&rev=a5c076ff1d57
With pristine make on windows:
https://tbpl.mozilla.org/?tree=Try&rev=d47772b849d5
With pymake:
https://tbpl.mozilla.org/?tree=Try&rev=2d74aa0ebcc1
Attachment #8340169 -
Flags: review?(mshal)
Attachment #8340169 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #8340163 -
Attachment is obsolete: true
Attachment #8340163 -
Flags: review?(mshal)
Attachment #8340163 -
Flags: review?(gps)
Assignee | ||
Comment 6•11 years ago
|
||
Another attempt:
Mozmake and all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=5c1db614bd50
Pristine make:
https://tbpl.mozilla.org/?tree=Try&rev=f3516fdc7d8b
Pymake:
https://tbpl.mozilla.org/?tree=Try&rev=db8fdc24e270
Attachment #8340177 -
Flags: review?(mshal)
Attachment #8340177 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #8340169 -
Attachment is obsolete: true
Attachment #8340169 -
Flags: review?(mshal)
Attachment #8340169 -
Flags: review?(gps)
Assignee | ||
Comment 7•11 years ago
|
||
I'm putting the gyp issues aside, they'll have their own bug because there is actually a problem with mozmake.py and subsequent problems in the gyp files themselves. They are only affecting building with pristine make. Not mozmake or pymake.
Attachment #8340268 -
Flags: review?(mshal)
Attachment #8340268 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #8340177 -
Attachment is obsolete: true
Attachment #8340177 -
Flags: review?(mshal)
Attachment #8340177 -
Flags: review?(gps)
Assignee | ||
Comment 8•11 years ago
|
||
Ms2ger tells me mochitests support subdirectories, so here we are, with a variant using a manifest.
Attachment #8340312 -
Flags: review?(mshal)
Attachment #8340312 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #8340148 -
Attachment is obsolete: true
Attachment #8340148 -
Flags: review?(mshal)
Attachment #8340148 -
Flags: review?(gps)
Comment 9•11 years ago
|
||
Comment on attachment 8340312 [details] [diff] [review]
Convert dom/imptests to INSTALL_TARGETS and editor/libeditor/html/tests to mochitest manifest
Review of attachment 8340312 [details] [diff] [review]:
-----------------------------------------------------------------
I would have gone with browserscope/**, but this is fine.
Attachment #8340312 -
Flags: review?(mshal)
Attachment #8340312 -
Flags: review?(gps)
Attachment #8340312 -
Flags: review+
Comment 10•11 years ago
|
||
Comment on attachment 8340268 [details] [diff] [review]
Replace double quotes with single quotes in Makefiles (or remove them when it makes sense)
>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
Is this (and other libs:: target removals) intended to be a part of this patch? They all look fine to me, but I don't see what that has to do with replacing double quotes with single quotes.
>diff --git a/testing/testsuite-targets.mk b/testing/testsuite-targets.mk
>-CHECK_TEST_ERROR_RERUN = $(call check_test_error_internal,"To rerun your failures please run 'make $@-rerun-failures'")
>+CHECK_TEST_ERROR_RERUN = $(call check_test_error_internal,'To rerun your failures please run 'make $@-rerun-failures'')
I think you want double-quotes around "make $@-rerun-failures" now. When passed through echo, it looks like all quotes get dropped in the new version.
Can you clarify what exactly the benefit of switching all of these quotes is? It isn't immediately obvious to me when looking at this bug or the blocked bugs.
Attachment #8340268 -
Flags: review?(mshal)
Attachment #8340268 -
Flags: review?(gps)
Attachment #8340268 -
Flags: review+
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Michael Shal [:mshal] from comment #10)
> Comment on attachment 8340268 [details] [diff] [review]
> Replace double quotes with single quotes in Makefiles (or remove them when
> it makes sense)
>
> >diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
> >--- a/testing/mochitest/Makefile.in
> >+++ b/testing/mochitest/Makefile.in
>
> Is this (and other libs:: target removals) intended to be a part of this
> patch? They all look fine to me, but I don't see what that has to do with
> replacing double quotes with single quotes.
Damn, I was supposed to move them in the other patch, but apparently missed them somehow. I'll do that on landing.
> >diff --git a/testing/testsuite-targets.mk b/testing/testsuite-targets.mk
> >-CHECK_TEST_ERROR_RERUN = $(call check_test_error_internal,"To rerun your failures please run 'make $@-rerun-failures'")
> >+CHECK_TEST_ERROR_RERUN = $(call check_test_error_internal,'To rerun your failures please run 'make $@-rerun-failures'')
>
> I think you want double-quotes around "make $@-rerun-failures" now. When
> passed through echo, it looks like all quotes get dropped in the new version.
Ah, nicely spotted.
> Can you clarify what exactly the benefit of switching all of these quotes
> is? It isn't immediately obvious to me when looking at this bug or the
> blocked bugs.
Mmm I thought I had written this somewhere in a bug... but can't find it.
When building with an unpatches gnu make 4.0, commands are run with sh -c "...", and it turns out msys bash doesn't handle shell quoting with double quotes right in this situation, although make does quote the content it passes to sh properly. This is why mozmake has a patch to enable the feature to make make write a script and execute it instead of doing sh -c. But the thing is that when using single quotes, msys bash doesn't fuck up.
Interestingly, this *also* fixes make l10n-check with mozmake (it's currently orange on birch).
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #11)
> > Is this (and other libs:: target removals) intended to be a part of this
> > patch? They all look fine to me, but I don't see what that has to do with
> > replacing double quotes with single quotes.
Just to clarify, this removes the double quotes in the foreach.
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Comment 14•11 years ago
|
||
Fixup addressing review comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/646b46467e86
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cfe7047da1a0
https://hg.mozilla.org/mozilla-central/rev/942d149a7c0c
https://hg.mozilla.org/mozilla-central/rev/646b46467e86
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Assignee | ||
Comment 16•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [qa-]
Comment 17•11 years ago
|
||
FYI, the libffi changes were accepted upstream.
https://github.com/atgreen/libffi/commit/c86d9b6cc6e16ee262844a33b40441374400758c
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•