fix quoting in win64.mak: add them only when invoking command

[rainers]

...not when setting variable. This avoids having to skillfully quote variables with spaces in them: use "CC=%cl64%" instead of "CC=\"%cl64%\""

Not sure how the existing misquoting was supposed to work. Using quotes with the command is agnostic to using / instead of \ as a path separator, too.

[dlang-bot]

Thanks for your pull request, @rainers!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2438"

[jacob-carlborg]

Should the other makefiles be quoted as well?

[rainers]

Should the other makefiles be quoted as well?

Do you mean win32.mak? IIRC DM tools don't work too well with spaces in path names to begin with.
For posix two mechanisms exist to deal with spaces, quotes and escaping with \ . I don't want to mess with that...

[rainers]

I removed auto-merege as I just noticed that I forgot to remove the quotes from the definition of CC and AR, which should be done for consistency. It will break patching by the auto-tester, though. I'll update here and hope for braddr/at-client#10 to go anywhere...

[thewilsonator]

OK.

[CyberShadow]

Oof, this broke Digger. Did not see this when it was posted. Will see if Digger can detect / patch around this.

[CyberShadow]

This is also unlike how it's done in Phobos and DMD:

https://github.com/dlang/phobos/blob/ccfe79be52ec12c0c22c783dac69d41b7fcd5c27/win32.mak#L70
https://github.com/dlang/dmd/blob/2ae5408b5923fc0d5e6ba3b9f292b449d0cae0c0/src/win32.mak#L447

Working around this will require special handling for this Druntime makefile specifically.

@rainers Would you mind going into a bit more detail for your motivations behind this change? Was it just some random cleanup opportunity you noticed, or does this PR facilitate some prior goal?

Edit: Found the Phobos PR (for win64.mak only as well): dlang/phobos#6825

[rainers]

@CyberShadow Sorry for the breakage. How does digger invoke make?

As you noticed this is for win64.mak only (it is also used to build model 32mscoff). I wouldn't expect dmc to work in a path with spaces to begin with, so there is little need to patch win32.mak.

The default path for VS installations contain spaces, though. The previous version of the makefiles was quite a mess: some paths were expected to be quoted, some not. Some were forwarded with pretty broken escaping, e.g. "AR=\$(AR)"\"" should have been "AR=\"$(AR)\"".

The basic idea of the change is to unify that and simplify specifiying arguments by "AR=<path with paces>", so the variables themselves never contain quotes. Being quoted when invoked also makes it agnostic to using / or \ in the path.

[CyberShadow]

@CyberShadow Sorry for the breakage.

No problem, Digger's goal is to deal with breakages like these throughout D's history.

How does digger invoke make?

I just finished implementing the workaround:
CyberShadow/ae@b4a0312

The default path for VS installations contain spaces, though. The previous version of the makefiles was quite a mess: some paths were expected to be quoted, some not. Some were forwarded with pretty broken escaping, e.g. "AR=\$(AR)"\"" should have been "AR=\"$(AR)\"".

Yes, combined with cmd.exe escaping it was quite messy:

digger: Running: "make" -f win64.mak ... ^"CC=\^"C:\...\cl.exe\^"^" ^"LD=\^"...

The basic idea of the change is to unify that and simplify specifiying arguments by "AR=<path with paces>", so the variables themselves never contain quotes. Being quoted when invoked also makes it agnostic to using / or \ in the path.

I think at some point for posix.mak the lack of quoting was actually abused to add switches to CC and such, but I don't think this is the case for the Windows makefiles.

[rainers]

I just finished implementing the workaround:

Thanks, looks good.

digger: Running: "make" -f win64.mak ... ^"CC=^"C:...\cl.exe^"^" ^"LD=^"...

Wow, that looks even worse than what I used until recently.

I think at some point for posix.mak the lack of quoting was actually abused to add switches to CC and such, but I don't think this is the case for the Windows makefiles.

Yeah, that's a possible abuse. I didn't need it, and I hope noone else did ;-)

[MartinNowak]

@rainers @CyberShadow @wilzbach this has also broken nightly builds since about 3 weeks.
https://buildkite.com/dlang/build-release/builds/205#68545f68-a853-475d-90ea-1ba78e471b62/6-9904
Here is the quoting in the build driver https://github.com/dlang/installer/blob/d4438767e7402a809b8b4c1afd6dfa32c4cd91f8/create_dmd_release/create_dmd_release.d#L406-L411

[CyberShadow]

Yes, you'll need to remove quoting for Phobos/Druntime makefile invocations.

BTW, your quote function doesn't look quite in line with how cmd / CommandLineToArgvW parse their input. I recommend refactoring command line building to work on string[] instead of concatenating strings. Since you use spawnShell, escapeShellCommand should be appropriate.

[rainers]

Sorry for the breakage. I was already wondering why the nightlies are missing... See dlang/installer#368

[MartinNowak]

Thanks for the fix :)