Crystal fails to build libraries that link using __DIR__ when there are spaces in the project path

[baseballlover723]

Bug Report

If there are spaces in the project path, for instance, if your user folder has a space in it and your project is in your user folder, then crystal will fail to link to certain libraries if they link using DIR/local/path (for instance https://github.com/kostya/myhtml/blob/master/src/myhtml/lib.cr#L5) since it will not escape the space and it will fail with something like

cc: error: /home/baseballlover723/crystal_spaces_repo/with: No such file or directory
cc: error: spaces/lib/myhtml/src/myhtml/../ext/modest-c/lib/libmodest_static.a: No such file or directory
Error: execution of command failed with code: 1: `cc "${@}" -o /home/baseballlover723/.cache/crystal/crystal-run-spec.tmp  -rdynamic -L/home/baseballlover723/.asdf/installs/crystal/1.1.0/bin/../lib/crystal/lib /home/baseballlover723/crystal_spaces_repo/with spaces/lib/myhtml/src/myhtml/../ext/modest-c/lib/libmodest_static.a -lpcre -lm -lgc -lpthread -levent -lrt -ldl`

I've created a minimal repo to reproduce this issue: https://github.com/baseballlover723/crystal_spaces_repo

$ crystal -v
Crystal 1.1.0 [af095d72d] (2021-07-14)

LLVM: 10.0.1
Default target: x86_64-unknown-linux-gnu

There are a few ways to handle this. First off, its probably more correct to link like @[Link(ldflags: "'#{__DIR__}/../ext/modest-c/lib/libmodest_static.a'")] instead of @[Link(ldflags: "#{__DIR__}/../ext/modest-c/lib/libmodest_static.a")] but a search though github (https://github.com/search?l=Crystal&q=%40%5BLink%28ldflags%3A&type=Code) shows that most people don't do that. This makes me feel like this is something that crystal should handle. I tried escaping the flags in the linker (https://github.com/baseballlover723/crystal/commit/c01788051a051250fb46fa6356e2a9da75783a2a) but this didn't work since sometimes we want to run a shell command (@[Link(ldflags: "llvm-config-3.6 --ldflags 2>/dev/null || llvm-config-3.5 --ldflags 2>/dev/null || llvm-config --ldflags 2>/dev/null")]) or there are other spaces that shouldn't be escaped (@[Link(ldflags: "#{__DIR__}/../ext/libwebview.a -lstdc++")]).

Also because of https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/codegen/link.cr#L36 we can't even do something like @[Link(ldflags: "#{Process.quote(__DIR__)}/../ext/modest-c/lib/libmodest_static.a")].

I'm not sure what the correct thing to do should be, but I feel like this is common enough that it would be nice if crystal took care of this, instead of making everyone quote their link flags. Perhaps something like __DIR_ESCAPED__ or a separate link variable if you intend to run a shell command, or maybe crystal could detect that __DIR__ is being called in a link and escape it somewhere the compiler. I'm not sure if this can be fixed without breaking backwards compatibility.

Another (partial) solution could be have __DIR__ not expand symlinks. Since I came across this while using Windows WSL, where my github projects are in my windows user folder (which has a space), but is symlinked in WSL so that it doesn't have the space in the path (pwd: /home/baseballlover723/github). In this setup __DIR__ returns the true path with the space in it, since it looks like it expands the symlink. Having __DIR__ not expand symlinks would also fix the problem for me (and would allow for easier workarounds), though it wouldn't truely fix the problem.

[straight-shoota]

I think the only thing that makes sense would be a simple plugin fix for any use of __DIR__ in a linker flags argument. Anything else that requires a special syntax (like __DIR_ESCAPED__ or Process.quote) isn't going to be much useful, because it would pretty much be equivalent to just wrapping the path in quotes explicitly. It's not 100% equivalent (Process.quote is safer) but it should be good enough for practical use cases.

Identifying __DIR__ in ldflags should be easy to do and we can add some special handling to that. Parsing the entire ldflags value would add a lot of complexity. But simply expanding __DIR__ in ldflags with Process.quote(__DIR__) could work. The result would look like "/path/to/current directory"/../ext/modest-c/lib/libmodest_static.a. I think that should be fine on any shell.

[straight-shoota]

This won't work however, if the entire path in ldargs is already quoted explicitly: @[Link(ldflags: "'#{__DIR__}/../ext/modest-c/lib/libmodest_static.a'"}. Similar to the status quo, this works if __DIR__ does not require quoting, but breaks if it does.

[baseballlover723]

if identifying __DIR__ in ldflags is easy, we could also consider just escaping the space with \ or ' (for windows). It might even be feasible to just search for the value of __DIR__ and do a simple replace with either quotes or escaping the space. But I think that still runs into issues with explicit quoting.

I wonder if maybe this would be easier if ldflags took an array of string instead of just a singular string. I also wonder if maybe its worth it to disallow quotes when using __DIR__, since it looks like no one really quotes __DIR__ in ldflags. If there are spaces if other parts then it would be on the library maintainer to escape those using their own quotes (explicitly excluding the __DIR__) or escaping the space with \. This with a note in the docs might be the best and least disruptive soluiton.

[straight-shoota]

Spaces are the most common issue in file paths requiring escaping, but not the only ones. I don't think we should go the way to try to escape these strings for the shell. It's just too much complexity.

I like the idea of using an array literal as value for ldflags. 👍 We could even consider enforcing that.

[baseballlover723]

even with arrays there are still issues with escaping so long as we also allow shell commands to be run, but its certainly a better place to be in.