943728 - Use single quotes for shell quoting instead of double quotes

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Convert a couple custom install rules using double quotes to INSTALL_TARGETS

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.

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Replace double quotes with single quotes in Makefiles (or remove them when it makes sense)

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))

Comment 4

[Mike Hommey [:glandium]]

(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 :(

Comment 5

[Mike Hommey [:glandium]]

Attachment: Replace double quotes with single quotes in Makefiles (or remove them when it makes sense)

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Replace double quotes with single quotes in Makefiles (or remove them when it makes sense)

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

Comment 7

[Mike Hommey [:glandium]]

Attachment: Replace double quotes with single quotes in Makefiles (or remove them when it makes sense)

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.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Convert dom/imptests to INSTALL_TARGETS and editor/libeditor/html/tests to mochitest manifest

Ms2ger tells me mochitests support subdirectories, so here we are, with a variant using a manifest.

Comment 9

[Gregory Szorc [: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.

Comment 10

[Michael Shal [:mshal]]

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.

Comment 11

[Mike Hommey [:glandium]]

(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).

Comment 12

[Mike Hommey [:glandium]]

(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.

Comment 13

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe7047da1a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/942d149a7c0c

Comment 14

[Mike Hommey [:glandium]]

Fixup addressing review comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/646b46467e86

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cfe7047da1a0
https://hg.mozilla.org/mozilla-central/rev/942d149a7c0c
https://hg.mozilla.org/mozilla-central/rev/646b46467e86

Comment 16

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/9ac7ed427cd2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

FYI, the libffi changes were accepted upstream.
https://github.com/atgreen/libffi/commit/c86d9b6cc6e16ee262844a33b40441374400758c