rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.11k stars 880 forks source link

Consider using symlinks for rustup proxies #4022

Open ChrisDenton opened 5 days ago

ChrisDenton commented 5 days ago

This was brought up elsewhere so I'm writing an issue here to note the conversation. I'm happy to implement this if it's acceptable (though I don't mind if someone else wants to claim it).

Currently rustup proxies are created using hard links with a fallback to soft links if creating hard links aren't supported by the filesystem or the platform (e.g. android). This proposal is to swap the order so that proxies are created using soft links with a fallback to hard links if creating soft links aren't supported by the filesystem or platform (e.g. Windows in the default configuration).

Hard links have the issue that they are, for most purposes, normal files. So many copy utilities (including cp) will just naively copy the file. This can be a problem when copying the rustup directory as all the hard linked proxies will be turned into literal copies of rustup.

Using symlinks for this use case is common in Unix-like environments. E.g. busybox is a canonical example. I have not been able to find any real downside to this approach (though I may have missed something).

rami3l commented 5 days ago

@ChrisDenton Thanks! I personally have no objection to this proposal, and would love to see experiments regrading switching to soft links.

cc @rbtcollins Just in case, do you have any historical context in terms of why we are doing hard links today?

rami3l commented 4 days ago

@ChrisDenton I've just been pointed out the original rationale:

Use hardlinks instead of softlinks for self installation / --link-local

Softlinks need admin access on windows, which is a pain. I think hardlinks should be fine since we are in control of all the sources and destinations. https://github.com/rust-lang/rustup/issues/88

This reminds me again of the fact that I'm still quite new to Windows development. Maybe things have changed since then, or you've correctly handled this case in #4023 already?

ChrisDenton commented 4 days ago

Creation of symlinks can be enabled for all users by turning on developer mode in settings. Which does require someone with admin access to do (though most users do have admin access on their own machine).

image

Alternatively an admin can assign the SeCreateSymbolicLinkPrivilege permission to a specific user or group.


From the above you can infer that this is not enabled by default but that's ok because if creating symlinks fails we fallback to using hardlinks. Also all users can still use or delete symlinks so it doesn't matter if another user creates the symlink (although running the same rustup for multiple users isn't a supported workflow in any case),

rbtcollins commented 3 days ago

From memory we also have some logic about detecting alterations that looks for the hardlink status of a proxy. So this may cause errors or failure to delete proxies on uninstall. I have no fundamental objection here, just will need some digging to be sure everything is caught.

ChrisDenton commented 3 days ago

Hm, I did see some code like that but it opens files by following symlinks and then uses same-file crate to compare with rustup. So I think that's unaffected. But I'll look into uninstall too.

Also do note that the symlink code path already existed (albeit as a fallback) so hopefully any such issues have already been addressed.

rami3l commented 2 days ago

@ChrisDenton https://github.com/rust-lang/rustup/pull/4023 has unfortunately caused regressions on FreeBSD:

> rustup self uninstall -y
  out.ok: false
  out.stdout (0 lines):

  out.stderr (4 lines):
      info: removing rustup home
      info: removing cargo home
      info: removing rustup binaries
      error: could not remove 'cargo_home' directory: '/home/runner/work/rustup/rustup/target/x86_64-unknown-freebsd/tests/running-test-xrCTlH/rustup-cargoDGW09M/ch': Too many links (os error 31)

https://github.com/rust-lang/rustup/actions/runs/10901896415/job/30252621940

Could this be a restriction with UFS2? [^1]

[^1]: See https://stackoverflow.com/a/22604322 for a similar syndrome.

ChrisDenton commented 2 days ago

Ooh interesting, "too many links" seems a bit weird considering we're now prioritising symlinks.

31 EMLINK Too many links. The number of hard links to a single file has exceeded the maximum. (The system-wide maximum number of hard links is 32767. Each file system may impose a lower limit for files contained within it).

I wonder why we're making so many hard links... I will investigate tomorrow.

ChrisDenton commented 2 days ago

Ah just saw your edit. Yeah that seems worth looking at.

ChrisDenton commented 1 day ago

Hm, so I'm confused why this is failing at removing directories, not creating proxies. As far as I can tell (I could be wrong, please correct me if so), removing the directory ultimately lands on https://github.com/rust-lang/rustup/blob/3c53cbc3fdd81bc18a198b1a1a61280680ec274e/src/utils/raw.rs#L244 which is the remove_dir_all crate. I'm not sure why that would be failing on FreeBSD when there are symlinks in the directory. cc @rbtcollins

I think this might be one of those cases where an errno is overloaded so it's error string is a bit misleading. The obvious workaround is to just always use hard links on FreeBSD for now.

ChrisDenton commented 1 day ago

Ooh, this might be relevant: https://man.freebsd.org/cgi/man.cgi?query=open&sektion=2&n=1#end. There's a mention that FreeBSD returns EMLINK instead of ELOOP. Which might be relevant here: https://github.com/XAMPPRocky/remove_dir_all/blob/4306a5f6b9f5a3ecd784349b333c33db7e1686b1/src/_impl.rs#L153