Don't canonicalize executable path

[Byron]

Otherwise symbolic links may also accidentally be resolved which may lead to unexpected results in the case of 'coreutils', a binary that depends on the executable name being a symbolic link.

This means a path like /bin/echo being canonicalized to /bin/coreutils will loose all information about the desired functionality.

For example, test failures will occur if 'echo' is resolved that way and it's not trivial to find the cause of it in the provided error messages. For exampledoc_workspace_open_different_library_and_package_names did fail for me on MacOS, Nix packages in PATH, but works with this patch.

With this patch, there is still the possibility that a path gets canonicalized for its relative path components, but still results in changing the name of the binary. I could imagine to check for binary name changes and panic if coreutils or busybox is encountered, which are known to fail without a symlink telling them which program to emulate.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[joshtriplett]

Might it make sense to get the directory name of the path, canonicalize that, then re-append the basename? That would allow unconditionally canonicalizing without affecting argv[0].

[Byron]

@joshtriplett I very much like the idea, and think it would be great to do that transformation right before trying to canonicalize the path. This means it would still avoid any canonicalization if there is seemingly no need due to the lack of relative path components.

If that sounds good, I can adjust the PR accordingly.

[alexcrichton]

Typically in Cargo we're very wary of the canonicalize function and try to use it as little as possible, so I was curious why this was used in this case.

It turns out this is somewhat of a storied history. From what I can tell the canonicalize() call was introduced in #3789 and AFAIK there was no discussion of it at the time, it was simply there. In #4634 when removing the reliance on current_exe that kept the calls to canonicalize() and propagated it to a few more places. Finally #5359 refactored to basically what it is today.

From looking at the history I don't know of any reason why we need to use canonicalize() here, and ideally we'd remove it entirely. I think everything should work here if we do current_dir().join(the_exe) instead of canonicalize since if the exe is a root path then that takes over, otherwise if it's relative then it's relative to the current directory.

[Byron]

@alexcrichton Great detective work! I have put it to the test, removing canonicalization entirely.

[joshtriplett]

@Byron There's another call to canonicalize a few lines later.

[joshtriplett]

@bors r+

[bors]

📌 Commit cf8e464 has been approved by joshtriplett

[bors]

⌛ Testing commit cf8e464 with merge e165bc8...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing e165bc8 to master...