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

Fix RUSTUP_PERMIT_COPY_RENAME condition so it is actually used #3292

Merged

Conversation

dylanahsmith
Copy link
Contributor

cc @mckaymatt

Problem

It looks like #2410 stopped working because it has a match on the io::ErrorKind::Other, but the experimental io::ErrorKind::CrossesDevices variant was later added, which prevented it from every matching the match arm for the RUSTUP_PERMIT_COPY_RENAME condition. As such, it would never fallback to copying and deleting and just continue failing with the "Invalid cross-device link" error.

Solution

An explicit match on io::ErrorKind::Other wasn't necessary (it already is conditional on the raw_os_error) and this bug showed how matching on it makes the code fragile to the addition to new variants, so I just replaced it with a _ catch all match with the same condition.

Testing

I've managed to manual test this in isolation, although the use of sudo to mount an overlayfs seems like it would be a problem with testing this through cargo test.

# dev setup as suggested in CONTRIBUTING.md
mkdir home
export RUSTUP_HOME=home CARGO_HOME=home
target/debug/rustup-init --no-modify-path --default-toolchain=none -y

# force download of an older version to test upgrading
home/bin/rustup toolchain install 1.68.1
mv home/toolchains/{1.68.1-x86_64-unknown-linux-gnu,stable-x86_64-unknown-linux-gnu}

# turn rustup home into an overlay over the initial rustup installation
mv home homelower
mkdir homeupper homework home
sudo mount -t overlay overlay -o lowerdir=homelower,upperdir=homeupper,workdir=homework home

RUSTUP_PERMIT_COPY_RENAME=1 home/bin/rustup update

# cleanup
sudo umount home
rm -r homework home homelower homeupper

On master, I get the "could not rename component file" error, but it works when retrying the above steps on this branch.

The addition of the experimental io::ErrorKind::CrossesDevices variant
prevented the RUSTUP_PERMIT_COPY_RENAME condition on io::ErrorKind::Other
from matching.  This shows why it is a bad idea to explicitly match on
io::ErrorKind::Other rather than using the default pattern and it doesn't
seem like there was any need to do that anyways given the raw_os_error
condition.
@rbtcollins
Copy link
Contributor

@hi-rustin letting you know I'm merging this in - its a regression fix, and super simple

@rbtcollins rbtcollins merged commit 7d6fe1a into rust-lang:master Mar 30, 2023
@dylanahsmith dylanahsmith deleted the fix-RUSTUP_PERMIT_COPY_RENAME branch March 30, 2023 16:44
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.

2 participants