Skip to content

refactor(install): decouple helpers from process then re-use the resolution result - #5096

Open
Cloud0310 wants to merge 6 commits into
rust-lang:mainfrom
Cloud0310:install-state
Open

Cloud0310 wants to merge 6 commits into
rust-lang:mainfrom
Cloud0310:install-state

Conversation

@Cloud0310

Copy link
Copy Markdown
Contributor

#5091 was not a complete decouple of Process's state from the related install/uninstall helpers.
So, this PR does:

  • renaming relevant fucntions for better expressing what they're doing
  • removing unnessary Cow usage of cargo_home_str_with_home, cargo_home_str, as the caller immediately constructs String
  • decouple Process' states, e.g. cargo_home and other, allowing for re-using the resolution result.
  • dedup remove_legacy_source_command, as the same logic is already done in do_remove_from_path

In the meantime, this the first of a series of refactors around uninstall/install/self-replacing logic.

@rustbot

This comment has been minimized.

@djc djc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most of this looks fine to me.

Decoupling helpers from Process seems pretty messy for ostensibly very limited benefit.

View changes since this review

@rami3l

rami3l commented Sep 21, 2026

Copy link
Copy Markdown
Member

@djc I would like to provide some extra context regarding this part of the changes: it has been a follow-up of my request #5091 (comment) in a previous attempt of decoupling the rcfile-related logic from &Process.

This is needed because the new uninstallation logic will attempt to remove the rcfile entries from both the CARGO_HOME/bin and CARGO_BIN_HOME installations in #5056, and thus these paths can no longer be injected from &Process anymore.

Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update.rs Outdated
@rustbot

This comment has been minimized.

Pass bin directories to binary, proxy, and updater helpers, and pass the resolved Cargo home to Windows uninstall GC. Give shell script helpers separate env and bin directory inputs while preserving legacy home-variable spelling.
Pass resolved env and user-home paths into source-line rendering. Share exact source-line removal and reuse the current operation paths and legacy rcfile candidates during cleanup.
@rustbot

rustbot commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Cloud0310

Cloud0310 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@rami3l seems there's an error upon CI/CD, and it's not due my code. Could you re-run it?

And leaving alone the part of Display, I've refactored the git history for cleanup.

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM modulo some tiny nits. Thanks again :)

View changes since this review

Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update.rs Outdated
Keep canonical_cargo_home and accept resolved paths without renaming it. Pass complete bin and env directory text to the templates, share post-install wording across platforms, and reuse the resolved homes for source commands. Keep display formatting separate from filesystem paths and use native separators in the uninstall message.
@Cloud0310

Copy link
Copy Markdown
Contributor Author

Fixed all of them @rami3l .

This branch has not been deployed

No deployments
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.

4 participants