externalToLink: use quoteShell to avoid issues with spaces in paths for {.link.} pragmas

[timotheecour]

• closes Add a note to windows users to avoid blanks #17857 by addressing the underlying bug
• remove some code duplication

/cc @m33m33 please try this PR
the logs mentioned in #17857 (comment) (https://pastebin.com/cnGfPz1X) showed

Hint:  [Link]
gcc.exe: error: C:\Nim: No such file or directory
gcc.exe: error: Lang\Nim\icons\nim_icon.o: No such file or directory
Error: execution of an external program failed: 'gcc.exe   -o "C:\Nim Lang\Nim\compiler\nim.exe"  C:\Nim Lang\Nim\icons\nim_icon.o "C:\Nim Lang\Nim\nimcache\r_windows_amd64\stdlib_assertions.nim.c.o"

which led to this bugfix

future work

• judging from the logs in https://pastebin.com/cnGfPz1X, looks like build_all.bat keeps going instead of aborting after 1st error:

"C:\Nim Lang\Nim\nimcache\r_windows_amd64\@mnim.nim.c.o"    '
FAILURE
...
bin\nim.exe c -o:bin\nimsuggest.exe -d:release -d:danger --skipUserCfg --skipParentCfg nimsuggest/nimsuggest.nim
Hint: used config file 'C:\Nim Lang\Nim\config\nim.cfg' [Conf]
Hint: used config file 'C:\Nim Lang\Nim\nimsuggest\nimsuggest.nim.cfg' [Conf]
Hint: used config file 'C:\Nim Lang\Nim\config\config.nims' [Conf]
Hint: used config file 'C:\Nim Lang\Nim\nimsuggest\config.nims' [Conf]
C:\Nim Lang\Nim\nimsuggest\config.nims(1, 2) Error: invalid command line option: '--filenames'
FAILURE

this is a bug, it should stop on 1st error; the problem is that bin\nim.exe wasn't properly built in 1st step, and then it reuses an old nim to build the rest (so it fails because '--filenames' is not available for older nim versions)

• simplify json generation instead of writing it manually (either with std/json or std/jsonbuilder) refs externalToLink: use quoteShell to avoid issues with spaces in paths for {.link.} pragmas #17875 (comment) => simplify extccomp.nim json logic via jsonutils; fix #18084 #18100
• there's still a bug which is bootstrap won't work if there's a space in dir until bootstrap version >= this PR ; until then we may need to change bootstrap logic to remove jsonscript (see proposal in externalToLink: use quoteShell to avoid issues with spaces in paths for {.link.} pragmas #17875 (comment))

[m33m33]

On a fresh git clone, I have overwritten extccomp.nim with this https://raw.githubusercontent.com/nim-lang/Nim/09701820f782ddcd8ef97eee2d31e75e2780dfa3/compiler/extccomp.nim but I still get the same error.

[timotheecour]

general comment: can you show text logs (and via links, like you did in https://pastebin.com/cnGfPz1X, or via github gists works too) instead of images?

did you rule out possibility that the old nimcache dir was used?

please add echo "jsonfile:", result right after result = getNimcacheDir(conf) / conf.outFile.changeFileExt("json")

and the show both the path being printed as well as the contents after the thing completes

(note that the change in this PR is desirable even if the problem still holds, there was a bug that's being fixed here)

[m33m33]

general comment: can you show text logs (and via links, like you did in https://pastebin.com/cnGfPz1X, or via github gists works too) instead of images?

Here it is for the previous build : https://pastebin.com/RHUGWbBh

did you rule out possibility that the old nimcache dir was used?

I deleted the old nim directory, an ran a new git clone, it clears the nimcache dir since it is a subdirectory.

please add echo "jsonfile:", result right after result = getNimcacheDir(conf) / conf.outFile.changeFileExt("json")

and the show both the path being printed as well as the contents after the thing completes

(note that the change in this PR is desirable even if the problem still holds, there was a bug that's being fixed here)

Not sure I really get what you're asking here, I added the echo in extccomp.nim at two places : extccomp-nim-L838 and extccomp-nim-L949

Here is the log I get on screen : https://pastebin.com/HHvmvwtn it still fails and the echo doesn't show. Maybe the problem happens earlyer than that ?

[m33m33]

I guess the issue is that the first object file (.o) is not quoted when passed to gcc for linking, when all the others are quoted:

Error: execution of an external program failed: 'gcc.exe -o "D:\Nim Lang\Nim\compiler\nim.exe" D:\Nim Lang\Nim\icons\nim_icon.o "D:\Nim Lang\Nim\nimcache\r_windows_amd64\stdlib_assertions.nim.c.o" "D:\Nim Lang\Nim\nimcache\r_windows_amd64

[timotheecour]

I guess the issue is that the first object file (.o) is not quoted when passed to gcc for linking, when all the others are quoted:

well ya, that's what prompted this PR; that file comes from {.link: "../icons/nim_icon.o".} which led to externalToLink. I think I understand what's going on: the bootstrap script runs from a nim binary compiled from csources that has the quoting bug, and if you follow the steps:

build_all
bin\nim.exe c  --skipUserCfg --skipParentCfg --nimcache:nimcache/r_windows_amd64 -d:release --skipUserCfg --skipParentCfg --compileOnly compiler\nim.nim
bin\nim.exe jsonscript --nimcache:nimcache/r_windows_amd64 -d:release --skipUserCfg --skipParentCfg compiler\nim.nim

the last step fails because we're still running the old bootstrap nim here (which contains the bug).

note that this PR is still correct but we also need to handle the bootstrapping step. please try this:

replace

    # in order to use less memory, we split the build into two steps:
    # --compileOnly produces a $project.json file and does not run GCC/Clang.
    # jsonbuild then uses the $project.json file to build the Nim binary.
    exec "$# $# $# --nimcache:$# $# --compileOnly compiler" / "nim.nim" %
      [nimi, bootOptions, extraOption, smartNimcache, args]
    exec "$# jsonscript --nimcache:$# $# compiler" / "nim.nim" %
      [nimi, smartNimcache, args]

by

    # in order to use less memory, we split the build into two steps:
    # --compileOnly produces a $project.json file and does not run GCC/Clang.
    # jsonbuild then uses the $project.json file to build the Nim binary.
    exec "$# $# $# --nimcache:$# $# compiler" / "nim.nim" %
      [nimi, bootOptions, extraOption, smartNimcache, args]

(ie, make sure it runs something like: bin\nim.exe c --skipUserCfg --skipParentCfg --nimcache:nimcache/r_windows_amd64 -d:release --skipUserCfg --skipParentCfg compiler\nim.nim, adjust as needed if i've introduced a typo).

this will skip the jsonscript logic for now to avoid this bug

[m33m33]

Ok with that last trick it works, the compilations ends with success.

Nimble.exe immediately gets deleted by the antivirus but that's another issue.

[timotheecour]

@Araq iirc you introduced jsonscript some time back, i think to limit memory consumption on low memory systems; as noted here #17875 (comment), the bugfix in this PR won't take effect until nim is bootstrapped, so we still need to address that problem; how about skipping jsonscript by default but allowing a flag -d:nimBootstrapJsonscript, so users can by default have something that works including with spaces (maybe even faster than with jsonscript intermediate step) and have an option to use jsonscript if they have tight memory requirements (and we just don't support the intersection of those 2 needs, which is rare, at least until csources is updated to >= 1.6)

[Araq]

I don't understand the connection ... we can always leave out the icon linking in the first bootstrapping iteration instead, resource constrained build environments are everywhere, I like to keep the existing default.

[timotheecour]

we can always leave out the icon linking in the first bootstrapping iteration instead

=> handling this slightly differently in #18337