Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try symlinking proxies first, falling back to hardlinking if that fails #4023

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

ChrisDenton
Copy link
Member

This reverse the previous order.

Closes #4022

Fallback to hardlinking if that doesn't work
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisDenton This PR looks good overall, I second @djc.

I have just one concern about what this would mean for existing users with existing hard links. Will this cause problems WRT upgrades?

@ChrisDenton
Copy link
Member Author

Oh, looking at this I did find one issue, The raw::hardlink function sneakily includes a load bearing let _ = fs::remove_file(dest); call whereas there's no symlink equivalent. I should probably move that to a higher level rather than it being so implicit.

Otherwise there shouldn't be an issue as far as I can tell from the install_proxies function. We use the same-file crate to test if two handles/fds point to the same file. And we open the handle by following symlinks so it'll work in either case.

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! I guess we can try this out as a part of the upcoming release.

@rami3l rami3l added this pull request to the merge queue Sep 17, 2024
Merged via the queue into rust-lang:master with commit d89f785 Sep 17, 2024
27 checks passed
@ChrisDenton ChrisDenton deleted the sym branch September 17, 2024 13:03
@rami3l rami3l mentioned this pull request Oct 6, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using symlinks for rustup proxies
3 participants