Skip to content

fix: display absolute path in the missing in PATH warning #16125

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

fix: display absolute path in the missing in PATH warning #16125

wants to merge 9 commits into from
+193 −18

Conversation

@0xPoe
Copy link
Member

@0xPoe 0xPoe commented Oct 16, 2025 •
edited
Loading

What does this PR try to resolve?

close #16023

Improve the warning message for realtive install location.

  1. Did a small refactoring to inline GlobalContext::get_path to prevent potential misuse in the future.
  2. Made Cargo always display the absolute install path in warnings.
  3. Added a warning indicating that install.root paths without a trailing backslash will be deprecated in the future.

How to test and review this PR?

Please check the unit test.

r?@ghost

ehuss
ehuss reviewed Oct 16, 2025
View reviewed changes
@0xPoe 0xPoe force-pushed the poe-patch-install.root branch 3 times, most recently from daf3b81 to 9e4b6b4 Compare October 27, 2025 20:06
@0xPoe 0xPoe force-pushed the poe-patch-install.root branch 4 times, most recently from b141907 to 7867550 Compare October 29, 2025 20:37
@0xPoe 0xPoe changed the title refactor: inline GlobalContext::get_path fix: display absolute path in the missing in PATH warning Oct 29, 2025
@0xPoe 0xPoe force-pushed the poe-patch-install.root branch from 8995ab9 to 34cc964 Compare November 2, 2025 21:25
@0xPoe 0xPoe marked this pull request as ready for review November 2, 2025 21:25
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 2, 2025
0xPoe
0xPoe commented Nov 2, 2025
View reviewed changes
Copy link
Member Author

@0xPoe 0xPoe left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback)

  • Code compiles successfully

  • Unit tests added

  • All tests pass

  • Comments added where necessary

  • Logs added where necessary

  • PR title and description updated

  • Documentation PR created (or confirmed not needed)

  • PR size is reasonable

View changes since this review

@0xPoe 0xPoe force-pushed the poe-patch-install.root branch 3 times, most recently from 080a4e7 to 24904ce Compare November 2, 2025 22:14
@0xPoe 0xPoe requested a review from ehuss November 2, 2025 22:16
weihanglo
weihanglo reviewed Nov 3, 2025
View reviewed changes
@0xPoe 0xPoe force-pushed the poe-patch-install.root branch from 24904ce to fb0690b Compare November 3, 2025 21:08
@rustbot rustbot added the A-configuration Area: cargo config files and env vars label Nov 3, 2025
@0xPoe 0xPoe requested a review from weihanglo November 3, 2025 21:30
@0xPoe 0xPoe force-pushed the poe-patch-install.root branch from cb41a3f to dc89d66 Compare November 3, 2025 21:37
weihanglo
weihanglo reviewed Nov 5, 2025
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

The final diff looks actually clearer than the commit history. Not sure the history was intended that way, though I feel like we might want a fixup.

Anyway, thanks for fixing this!

View changes since this review

tests/testsuite/install.rs
Comment on lines +542 to +543
fs::remove_file(root.join(".cargo/config.toml")).unwrap();
fs::remove_dir_all(&project_t1).unwrap();
Copy link
Member

@weihanglo weihanglo Nov 5, 2025

Choose a reason for hiding this comment

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

This mutates the test state. Should we have it as a separate test instead?

tests/testsuite/install.rs
}

#[cargo_test]
fn relative_install_location_with_path_set() {
Copy link
Member

@weihanglo weihanglo Nov 5, 2025

Choose a reason for hiding this comment

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

Would you mind having this test created before the fix? Otherwise it is a bit hard to tell where there was a bug and we are really fixing it.

src/cargo/ops/common_for_install_and_uninstall.rs
let definition = p.value().definition.clone();
if matches!(definition, Definition::Path(_)) {
let suggested = format!("{}/", resolved.display());
let notes = vec![
Copy link
Member

@weihanglo weihanglo Nov 5, 2025

Choose a reason for hiding this comment

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

array should be good enough,
and later no need to call into_iter()?

Suggested change
let notes = vec![
let notes = [

src/cargo/ops/cargo_install.rs
Comment on lines +668 to +674
let dst_path = root.join("bin").into_path_unlocked();
let cwd = gctx.cwd();
let dst = if dst_path.is_absolute() {
paths::normalize_path(dst_path.as_path())
} else {
paths::normalize_path(&cwd.join(&dst_path))
};
Copy link
Member

@weihanglo weihanglo Nov 5, 2025

Choose a reason for hiding this comment

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

nit: This function is too long and I am afraid of misuse of dst_path again. Perpahs we call it dst as well to avoid that?

Suggested change
let dst_path = root.join("bin").into_path_unlocked();
let cwd = gctx.cwd();
let dst = if dst_path.is_absolute() {
paths::normalize_path(dst_path.as_path())
} else {
paths::normalize_path(&cwd.join(&dst_path))
};
let dst = root.join("bin").into_path_unlocked();
let cwd = gctx.cwd();
let dst = if dst.is_absolute() {
paths::normalize_path(dst.as_path())
} else {
paths::normalize_path(&cwd.join(&dst))
};

src/cargo/ops/cargo_install.rs

let dst = self.root.join("bin").into_path_unlocked();
// `dst` is usually absolute; if not, make it absolute so messages are clearer.
// See: https://github.com/rust-lang/cargo/issues/16023
Copy link
Member

@weihanglo weihanglo Nov 5, 2025

Choose a reason for hiding this comment

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

The two duplicate make-absolute code actually fix two different warnings, should we make one commit for each to make the intent more obvious?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weihanglo weihanglo weihanglo left review comments

@ehuss ehuss Awaiting requested review from ehuss

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

A-configuration Area: cargo config files and env vars Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive "be sure to add .local/bin to your PATH" warning with relative install root

4 participants

@0xPoe @ehuss @weihanglo @rustbot