Closed Bug 943728 Opened 11 years ago Closed 10 years ago

Use single quotes for shell quoting instead of double quotes

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

      No description provided.
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)
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)
Depends on: 944558
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)
(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 :(
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)
Attachment #8340163 - Attachment is obsolete: true
Attachment #8340163 - Flags: review?(mshal)
Attachment #8340163 - Flags: review?(gps)
Attachment #8340169 - Attachment is obsolete: true
Attachment #8340169 - Flags: review?(mshal)
Attachment #8340169 - Flags: review?(gps)
Blocks: 944569
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)
Attachment #8340177 - Attachment is obsolete: true
Attachment #8340177 - Flags: review?(mshal)
Attachment #8340177 - Flags: review?(gps)
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)
Attachment #8340148 - Attachment is obsolete: true
Attachment #8340148 - Flags: review?(mshal)
Attachment #8340148 - Flags: review?(gps)
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 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+
(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).
(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.
Depends on: 945750
Depends on: 945978
Depends on: 946605
Whiteboard: [qa-]
Blocks: 990739
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.