Use single quotes for shell quoting instead of double quotes

RESOLVED FIXED in mozilla28

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla28
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
No description provided.
Assignee

Comment 1

6 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

6 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

Updated

6 years ago
Depends on: 944558
Assignee

Comment 3

6 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

6 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

6 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

6 years ago
Attachment #8340163 - Attachment is obsolete: true
Attachment #8340163 - Flags: review?(mshal)
Attachment #8340163 - Flags: review?(gps)
Assignee

Updated

6 years ago
Attachment #8340169 - Attachment is obsolete: true
Attachment #8340169 - Flags: review?(mshal)
Attachment #8340169 - Flags: review?(gps)
Assignee

Updated

6 years ago
Blocks: 944569
Assignee

Comment 7

6 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

6 years ago
Attachment #8340177 - Attachment is obsolete: true
Attachment #8340177 - Flags: review?(mshal)
Attachment #8340177 - Flags: review?(gps)
Assignee

Comment 8

6 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

6 years ago
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+
Assignee

Comment 11

6 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

6 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

Updated

6 years ago
Depends on: 945750
Depends on: 945978
Assignee

Updated

6 years ago
Depends on: 946605

Updated

6 years ago
Whiteboard: [qa-]
Blocks: 990739

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.