Remove use of hard links to symlinks on macOS.

[brotskydotcom]

Fixes #3136 (macOS only) by checking to see if the source path being linked to is itself a symlink, and using a symlink rather than a hard link if it is.

Per the contributing guidelines:

• all tests pass except for update_exact, which I can't see how to make work given this has a development version.
• there are several nightly lints that fail, but they are all in parts of the code I haven't touched.
• rustfmt is happy with the few lines of new code.

I have field tested this by using it as the binary for homebrew's rustup-init, which places a symlink for rustup.

This is a first submission to the rust-lang tree for me. All comments welcome.

[kinnison]

Hi @brotskydotcom

Thank you for trying to submit a fix. The shellcheck issue is definitely not your fault.

I think this change is acceptable since it does fix a long-standing issue with homebrew, although I'd also like to see a small patch to the rustup-init side of things to detect that it is being launched from a symbolic link and to warn the user that this may mean they will end up with confusion as to versions since they appear to be using a package manager to manage the installer for a package manager. If you don't feel up to the latter, then don't worry we can consider adding it later.

In a development tree, all the tests should always pass. Given they're clearly passing in CI (e.g. https://github.com/rust-lang/rustup/actions/runs/3861468865/jobs/6582462931#step:14:1032 ) I'd like to know how they're failing locally for you.

Finally I'd like to know how this behaves updating from a version of rustup which put symlinks to hardlinks in place to a version with this fix.

Rant about homebrew

What this change does is to mask a bug in homebrew's approach to packaging Rust. Specifically they should not be packaging rustup in this manner and despite attempts to correct this there has been no traction.

Realistically homebrew should be packaging only rustup-init.sh or they should be properly packaging rustup along with all the proxy links, and building said rustup with the no-self-update feature. I can understand that you wouldn't want to do that yourself though.

[brotskydotcom]

Hi @kinnison,

Thanks so much for the helpful review comments! Let me address your concerns one at a time...

I'd also like to see a small patch to the rustup-init side of things to detect that it is being launched from a symbolic link and to warn the user that this may mean they will end up with confusion as to versions since they appear to be using a package manager to manage the installer for a package manager.

I'm happy to work on this, although I'll admit I'm going to have to do some research to figure out how to tell whether the current process was launched from a symbolic link. Any tips you have in this regard would be welcome, as well as any suggested wording for the warning. Since you seem willing to have it come in separately from this fix, I will work on it in a separate PR, so as not to confuse this conversation.

In a development tree, all the tests should always pass. Given they're clearly passing in CI (e.g. https://github.com/rust-lang/rustup/actions/runs/3861468865/jobs/6582462931#step:14:1032 ) I'd like to know how they're failing locally for you.

After some investigation (which would have been made unnecessary if I had carefully read the entire contributing doc 🤕 ), I found that the version number comes from src/cli/common.rs which uses git_testament. So I tagged my local branch with the package version and now all the tests pass on my local machine. It's kind of interesting that they worked in CI - perhaps CI does the tagging or uses a trusted branch?

Finally I'd like to know how this behaves updating from a version of rustup which put symlinks to hardlinks in place to a version with this fix.

I'm not quite following. If you mean "hardlinks to symlinks" rather than "symlinks to hardlinks", then I think I understand and I can test that.

[kinnison]

Normal CI isn't using trusted branches, so it's a little odd that you needed that git tag, but so long as you can test locally I'm not too worried.

Yes I meant as you understood - it's the "upgrade" process of converting from hardlinks to symlinks -> symlinks to symlinks which needs validation since our test-suite as it stands can't do that check.

[brotskydotcom]

As far as I can tell, there is no scenario where a self update of an existing install will find hard links to a symlink and then need to replace them, because self update of existing installs always starts by first downloading and then copying the updated rustup executable into place, and that download will never produce a symbolic link, so there will be no need for the other files to be symlinks at all.

This is one of the reasons why homebrew's rustup-init is built with the no-self-update feature: if it self-updated then .cargo/bin/rustup would no longer be a symlink to /opt/homebrew/bin/rustup-init. (I have tested this using a non no-self-update build and you end up with the downloaded rustup replacing the symlink.)

What I have tested is running homebrew's rustup-init sequence with a no-self-update build of this PR, and that works just fine:

• homebrew installs the rustup-init executable in /opt/homebrew/Cellar/rustup/.../bin/rustup-init.
• homebrew puts a symlink at /opt/homebrew/bin/rustup-init pointing at the executable.
• the user executes rustup-init.
• the shell finds the symlink on the path and launches the executable from there.
• the executable finds that it was launched from a symlink, so (at utils::copy_file line 349 - line 357 in the PR) instead of copying the symlink to .cargo/bin it lays down a symlink to it.
• at that point the patched code comes into effect and creates symlinks to the newly-created symlink (instead of hard links to it).

Thus, this patch preserves the initial install behavior (in copy_file) that homebrew relies on, and just fixes the hard links currently created to be symlinks instead (because the installed rustup is a symlink).

On another topic, my research into how this all works is making me think that the additional fix you asked for:

I'd also like to see a small patch to the rustup-init side of things to detect that it is being launched from a symbolic link and to warn the user that this may mean they will end up with confusion as to versions since they appear to be using a package manager to manage the installer for a package manager.

is not actually necessary. There is nothing wrong with launching rustup-init from a symlink when you are doing an initial install. But if a package manager (such as homebrew) works in this way, it had better use a no-self-update build of rustup, because otherwise the self update will replace the package manager's version with the latest version from rust-lang. And if the package manager does use a no-self-update build, then it already will give the message you mention about "you're using a package manager build; use your package manager to update it."

@kinnison Your thoughts?

[brotskydotcom]

@kinnison Just checking in about this PR. Is there anything you want me to do to make it merge-ready?

[rbtcollins]

@kinnison is busy at the moment.

What I have tested is running homebrew's rustup-init sequence with a no-self-update > build of this PR, and that works just fine:

homebrew installs the rustup-init executable in /opt/homebrew/Cellar/rustup/.../bin /rustup-init.
homebrew puts a symlink at /opt/homebrew/bin/rustup-init pointing at the executable.
the user executes rustup-init.
the shell finds the symlink on the path and launches the executable from there.
the executable finds that it was launched from a symlink, so (at [utils::copy_file line 349](https://github.com/rust-lang/rustup/blob/master/src/utils/utils.rs#L349) - line 357 in the PR) instead of copying the symlink to .cargo/bin it lays down a symlink to it.

To my mind the failure is earlier: when rustup-init runs, it should realise it was launched from a symlink, resolve the symlink, and hard link to the target. Is there any reason not to do that? [If the answer is that homebrew cannot guarantee same-filesystem semantics, we could fall-back to copying the binary - but then you'd need a self-updating install. Which we're generally in favour of here.]

Another variation would be to symlink directly to the actual binary, not symlink to symlink.

Rustup uses hard links to keep page cache pressure and startup times low, symlinks aren't a huge problem - but the proxies do get called a lot in a cargo run. We're working on bypassing them safely to cut down overheads, so the hardlink or symlink thing will be less relevant soon I hope.

Ultimately the question is where do we - homebrew folk + rustup folk want to get to. As @kinnison says, we're not feeling listened to. But perhaps we're not listening well enough either.

[brotskydotcom]

@rbtcollins Thanks much for responding while @kinnison is busy.

With all due respect, I think maybe rust-lang is overthinking this one special case. Here's a different way of looking at the current situation:

• On Android, which doesn't allow hardlinks at all, symlinks to rustup are created in all cases.
• On Mac, the rustup code uses hardlinks unless their creation fails. Although hardlinks to symlinks can be created on some Mac filesystems, they aren't supported by Apple (there are Radar bugs about this marked wontfix) and lead to problems (e.g., the resulting directory structure cannot be copied). Thus rustup should not use hardlinks to symlinks on Mac. That's the change this PR makes.

This logic is completely independent of the (rigid) homebrew convention that hardlinks/executables are never to be placed in per-user directories. If rustup were to change to notice that it was a symlink and find the actual rustup executable, it would end up hardwiring the user's cargo directory to the homebrew Cellar executables in violation of all homebrew rules. A homebrew update of the rust toolchain would then break the user's install.

A homebrew install of rustup with this PR in place will work "just fine" (although fractionally less performant than before). But the current homebrew install (without this PR) can't be backed up or copied across filesystems because of Apple's lack of support for hardlinks to symlinks. I believe that's a more severe problem than the performance penalty this PR introduces. And users who are concerned about performance can always use the official rustup install instead of homebrew's package-managed install (which already has a double-symlink in every lookup).

Hope this helps clarify my thinking.

[rbtcollins]

Could you please rebase this, the failing tests have been fixed in the interim, but I'd just like to be sure. Also, 'all due respect'? /shakes head

[brotskydotcom]

Happy to rebase! And I can't quite tell if you were amused or upset by "all due respect," but I am an absolute newbie at rust-lang contributions and I have tremendous respect for the rust language maintainers, so I meant that very sincerely!

[rbtcollins]

I wasn't sure of the intent :) - in some cultural backgrounds, it is often a cutting sarcastic remark. In others it is a genuine superlative. Thanks for clarifying :)

[brotskydotcom]

I have rebased on top of current master and all local tests pass on Mac (the only platform with a code change). I did notice that there's a new clippy warning on Windows about doing a Box::new(_) of a default value but that's not me :).