Allow update to proceed even if a file/dir in the old toolchain is missing

[Walnut356]

Resolves: #1480

If a file doesn't exist, the outcome is identical to deleting it (from the user's perspective), so it shouldn't prevent rustup update from proceeding.

[rami3l]

@Walnut356 Hi, thanks for the patch!

If I have understood the context correctly, you are trying to provide a band-aid solution here to #988 and #4400 (many thanks for providing the link to #1480, I'll merge those issues on my side).

However, the underlying issue here is that rustup allows putting toolchains into a half-installed state and it will stay visible but any modifications to it might get stuck, which is exactly the kind of thing we would like to prevent from now on.

A more comprehensive solution is being worked on and tracked by rust-lang/goals#736. As such, I would reject this patch as-is. Many thanks nonetheless for the investigation!

[rami3l]

@Walnut356 Please don't hesitate to let me know if you have further concerns or if you want to help with triaging/issue resolution etc :D

[Walnut356]

However, the underlying issue here is that rustup allows putting toolchains into a half-installed state and it will stay visible but any modifications to it might get stuck, which is exactly the kind of thing we would like to prevent from now on.

While this might be an underlying issue, it's not the only one. I encountered this update failure because I intentionally modified my toolchain install (see: rust-lang/rust#162042 (comment)). i.e. my toolchain was in a "broken" state as a direct result of manual intervention long before i ever ran rustup update.

FWIW, in the context of debugger visualizers, it should be valid for users to overwrite our existing ones with their own (including ones that don't match our existing file structure). Since they are scripts loaded at debug-time by the debugger, it is expected that users can modify them when/however they want to. Modifying them in the toolchain install makes things significantly easier since most tools (e.g. CodeLLDB) already point there. Anecdotally, I pull mine from rust/main, both to get the new features in advance, and when testing changes for patches.

Regardless, I disagree that this is a bandaid. I don't think it makes any sense for rustup update to unilaterally fail simply because a file it wants gone is already gone.

[rami3l]

@Walnut356 LLDB supports injecting debug scripts from any path, so you don't need to modify the one in the installation. See https://github.com/cmrschwarz/rust-prettifier-for-lldb for an example of how you would use this with LLDB CLI or CodeLLDB. If you are using other IDEs than VSCode, say Neovim, they basically also support VSCode's configuration format, so it remains quite portable.

Otherwise, it looks like we are having a disagreement WRT rustup's fundamental assumptions here. Personally, I would argue that intentionally modifying the toolchain for e.g. custom debug scripts is not a supported use case, since rustup is assumed to have full control over all the files that it has installed (not linked toolchains which are designed to be immutable) and thus it's rustup's responsibility to ensure their integrity (admittedly, rustup has been historically doing a bad job here, but this is what we want to improve in the upcoming months). This assumption is also very important if we want to implement toolchain deduplication.

[Walnut356]

FWIW, I've been the one primarily updating rust's debugger visualizers and the debug info test suite for the past year or so, so I'm not unaware of the alternatives. That said, I have to test on more than 1 debugger adapter and it's way easier to change 1 toolchain once than a bunch of tools every single time i need to test something. I understand that's not a "default usecase", or even a common one, but i still don't think this failure mode makes any kind of sense.

Part of the issue is that command script import doesn't overwrite anything, it simply loads another script into the existing python context. If you have conflicting visualizers, the newer one gets used (on a type-by-type basis), but if you're manually changing the files like this, you generally don't want any of the builtin visualizers at all.

When I say tools "point to" the toolchain installation, i mean they automatically import rust's scripts and (at best) you need to manually tell them not to (which isn't always possible, e.g. if the command is built into a pre-compiled debugger adapter binary). For example, if I want to prevent CodeLLDB from importing rust's visualizers (so that I can use my own), I would need to modify this file in my vscode installation of CodeLLDB. Or I would need to incorporate manually unloading all of the rust visualizers in my own visualizer scripts.

There are instances of the visualizer scripts causing LLDB to segfault or read arbitrary amounts of ram (e.g. i've seen it try to read 1 << 63 bytes before) when encountering certain types in certain circumstances. The scripts were poorly maintained and poorly tested, and that's only recently been improving. I have absolutely recommended users replace, update, or temporarily rename these files in their toolchain since the alternative is not being able to use their preferred debugger adapter at all.

Regardless of all that though, as I said, I do not understand how this failure mode is useful. My understanding is that rustup update has 3 observable effects in the context of, e.g. stable-<target triple>:

1. download the new toolchain
2. copy the existing toolchain to <version>-<target triple>
3. put the new toolchain in stable-<target triple>

I fail to see how a missing file impedes any of those operations in any meaningful way. If the problem is that <old toolchain> is in a "broken" state, that is irrelevant to the update that the user asked for. It can be handled for the old toolchain after it has been relocated and the new toolchain has been installed. It should not prevent users from updating to the latest version, because a missing file in the old toolchain cannot effect the new toolchain. Consider that the reason they might be running rustup update in the first place is to "fix" their default toolchain (i.e. replace it with a brand new one that should work properly).

At the absolute most, rustup should emit a warning for the missing file and create <version>-<target triple> anyway. If the user wants to fix their old toolchain, they can. If not, they are still able to update. If it needs to be locked behind a y/n prompt or the --force flag, that's fine, but it should not be impossible for rustup update to succeed simply because a file it was already going to delete doesn't exist.

[Walnut356]

For sake of comparison, I think it makes a lot of sense to cancel update if there is a file that cannot be deleted due to insufficient permissions, since pasting the new toolchain overtop of that would leave a vestigial file. A non-existent file cannot cause any observable problem for the new toolchain in stable-<target triple>, so what purpose is there in preventing it?

[rami3l]

@Walnut356 Thanks a lot for the extra context here, and my sincere apologies for the premature closing of your patch!

I think I now have a better understanding of your situation.

On the other hand, since rustup notes all files it has installed, of course this will cause a conflict, and it will feel very weird to ask the user to modify both the record of the files being installed and the actual files on disk...

I definitely agree that the current failure mode is unacceptable if modifying the toolchain's contents is absolutely necessary like you have suggested already, where your proposed solution is pretty valid.

That said, I may need some more time to evaluate how that will fit into our common understandings of what rustup should take care of while satisfying your particular needs.

[rami3l]

For sake of comparison, I think it makes a lot of sense to cancel update if there is a file that cannot be deleted due to insufficient permissions, since pasting the new toolchain overtop of that would leave a vestigial file. A non-existent file cannot cause any observable problem for the new toolchain in stable-<target triple>, so what purpose is there in preventing it?

FWIW the original rationale of this failure mode is that rustup needs to backup everything it will be overwriting in case the whole transaction fails, and for that it has to know what exactly it has installed previously and if for some reason this record is inaccurate it will be considered an internal fault. However I believe this transaction model has serious design flaws.

I'm not opposed to introducing a fix like this right now if there are no other alternatives, but since the whole system will be replaced in a later version, I'm afraid this expected user journey will break some invariants (meaning this solution might work for you right now but break again in a later iteration, which both of us don't want to see.)

[Walnut356]

It's all good. My usecase is incredibly uncommon and definitely spacebar heating territory. I'm sure you'd be horrified to learn that on my other computer, my <stable>/lib/rustlib/etc has multiple test crates (all build artifacts included), several folders of in-progress visualizer rewrites, and a bunch of ide/python config folders. It ends up convenient because rustup update doesnt disturb any additional files that happen to be in the toolchain.

I'm not super attached to that workflow, especially as rust's tests have become more useful. My larger issue is <old toolchain> being in a bad state before rustup update is run should not prevent <new toolchain> from being installed in its place.

I'm absolutely not familiar with rustup or filesystems, but i was somewhat surprised to eee that the behavior involved deleting anything in the first place. Would it make more sense to just rename the top level stable-<target> directory to <version>-<target> and then just create a brand new stable-<target> directory? I guess that assumes update operation doesnt make any changes to anything inside the old toolchain (therefore there's not really a benefit to making a copy), but i'm not sure if that's the case.

[djc]

I'm not super attached to that workflow, especially as rust's tests have become more useful. My larger issue is <old toolchain> being in a bad state before rustup update is run should not prevent <new toolchain> from being installed in its place.

I agree that that seems like a useful invariant to maintain.

[rami3l]

I'm absolutely not familiar with rustup or filesystems, but i was somewhat surprised to eee that the behavior involved deleting anything in the first place. Would it make more sense to just rename the top level stable-<target> directory to <version>-<target> and then just create a brand new stable-<target> directory? I guess that assumes update operation doesnt make any changes to anything inside the old toolchain (therefore there's not really a benefit to making a copy), but i'm not sure if that's the case.

@Walnut356 That is a fair point, fully content-addressable Rust toolchains (at least official ones) is actually something I'm aiming at, but the current semantics is far from it. stable just means a single folder in the toolchain dir named stable-something, and every upgrade happens in-place, there is no such thing as <version>-<target> unless you have asked for it explicitly (the backup is a separate thing).

So just for the sake of exploration, let's say your 1.78 points to some 1.78.1 (I've made that up just for this example). My goal is that at the end of the 2nd stage of rust-lang/goals#736, if rustup sees that stable also points to 1.78.1 and that you are not asking for any more components, then it will point your stable to this shared installation. Continuing that logic you will see your manual changes made on 1.78.1 appear when you use either 1.78 or stable... Would that be surprising to you (this could mean rustup may think about toolchains as sharable when it actually can't be shared because you have manually changed what's in them)?

[Walnut356]

stable just means a single folder in the toolchain dir named stable-something, and every upgrade happens in-place, there is no such thing as - unless you have asked for it explicitly (the backup is a separate thing).

Hmm... I might be losing my mind 🫠 Maybe I have some config setting set or I'm forgetting how i'm running update or something? To clarify what I mean, i could have sworn the following was the case: if install stable back when stable is 1.90-x86_64-pc-windows-msvc, it lives at stable-x86_64-pc-windows-msvc. If i then run rustup update (current stable is 1.98), the result in my .rustup/toolchains folder is the following:

• .rustup/toolchains/stable-x86_64-pc-windows-msvc/ <-- which is 1.98 internally (and doesn't touch any files i added that it doesn't expect to be there)
• .rustup/toolchains/1.90-x86_64-pc-windows-msvc/ <-- which is a copy of what used to be in the stable-x86_64-pc-windows-msvc folder, minus anything I added to it, since it seemingly only copies the files it expects to exist. I figured it kept it around for cargo.lock reasons or something

But i checked the computer that i just updated and there isn't a .rustup/toolchains/1.94-x86_64-pc-windows-msvc/.

Maybe before i was using rustup toolchain install stable-msvc or rustup update stable and the behavior isn't identical between them? I've definitely had to clean out my toolchains because i had a bunch of old versions hanging around. Like my current rustup toolchain list output on my desktop is is:

stable-x86_64-pc-windows-gnu
stable-x86_64-pc-windows-msvc (active, default)
nightly-x86_64-pc-windows-msvc
nightly-2024-07-02-x86_64-pc-windows-msvc
nightly-2024-08-09-x86_64-pc-windows-msvc
nightly-2024-08-11-x86_64-pc-windows-msvc
nightly-2024-11-09-x86_64-pc-windows-msvc
nightly-2025-01-05-x86_64-pc-windows-msvc
nightly-2025-01-12-x86_64-pc-windows-msvc
nightly-2025-01-20-x86_64-pc-windows-msvc
1.76.0-x86_64-pc-windows-msvc
1.86-x86_64-pc-windows-msvc
1.90-x86_64-pc-windows-msvc
1.91-x86_64-pc-windows-msvc
stage0
stage1
stage1gnu
stage1host
stage2

And there's absolutely no way i would run such a hyper specific install (rustup toolchain install nightly-2025-01-20-x86_64-pc-windows-msvc) rather than rustup update nightly or rustup toolchain install nightly

[rami3l]

And there's absolutely no way i would run such a hyper specific install (rustup toolchain install nightly-2025-01-20-x86_64-pc-windows-msvc) rather than rustup update nightly or rustup toolchain install nightly

@Walnut356 That is what I am suggesting: this can never happen today unless you have asked for them specifically. rustup update nightly installs to nightly-<your-host-tuple>. Full stop.

However maybe the ask wasn't explicit on your side because it's hidden in some rust-toolchain.toml of a project you have interacted? Otherwise I have no idea why this could have happened...

[rami3l]

@Walnut356 I gave another look at your original issue.

A quick recap of what we have now: rustup only knows from the manifest files (the toolchain and component manifests respectively) what can be installed and what it has installed on the disk. Without any extra mechanisms that may be introduced in the future such as #3940, that is all rustup knows about a toolchain installation.

I'd love to accept this PR if you think the below is acceptable in terms of general user journey:

• If you have broken any of the invariants rustup internally assumes, it will perform a best effort uninstalling operation (for example succeed in uninstalling the conflicting toolchain/component), even if this means in the current transactional semantics rustup may (upon rollback) restore a toolchain installation that ends up broken (potentially even "more broken" than before because the files that are not registered in the manifest files are not backed up).

• In the future with Process-Safe Rustup Toolchain Operations goals#736, a manually modified toolchain may be reused depending on the manifest files alone, even if this means newly-installed toolchains that are deemed identical to your modified toolchain may appear as corrupted as your original one.

[rami3l]

IMHO we should keep the API shape (with component: &str). The actual error and this comment line can be converted to warn!() though, where component and path can still be useful.

Same thing for remove_dir() below.

View changes since the review

[rustbot]

☔ The latest upstream changes (possibly #5052) made this pull request unmergeable. Please resolve the merge conflicts.