Skip to content

For toolchain binaries use the full path found in $PATH #16755

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

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Mar 5, 2024

Fixes #16754

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2024
@Veykril
Copy link
Member Author

Veykril commented Mar 5, 2024

@MabezDev this reverts your recent PR, but if I understood the issue you had correctly, my changes in here should not break your workflow again (we now invoke the full path found in $PATH instead of just the binary name if we find a working path in $PATH).

@Veykril Veykril changed the title For toolchain binaries ue the full path found in $PATH For toolchain binaries use the full path found in $PATH Mar 5, 2024
@MabezDev
Copy link
Contributor

MabezDev commented Mar 5, 2024

Unfortunately, this does break my workflow. I checked out this PR and installed locally, replacing my binary which contained my original PR.

2024-03-05T13:29:59.280330Z ERROR rust_analyzer::main_loop: FetchBuildDataError:
error: package `block-device-adapters v0.1.0 (/home/mabez/development/rust/embedded/util/embedded-fatfs/block-device-adapters)` cannot be built because it requires rustc 1.75 or newer, while the currently active rustc version is 1.74.1

It's still trying to use my system Rust, and not the rustup installation.

@Veykril
Copy link
Member Author

Veykril commented Mar 5, 2024

That's odd, you said " My $PATH has $HOME/.cargo/bin before /usr/bin." in your reported issue before, so this should pick that up. Do you have the RUSTC env var set for some reason? No I guess that wouldn't make sense given your earlier PR

@Veykril
Copy link
Member Author

Veykril commented Mar 5, 2024

Oh wait, you literally have $HOME/.cargo/bin in your path I assume? Is that allowed ? If yes then the logic here won't resolve env vars in there so that would explain the issue but I doubt it is

@MabezDev
Copy link
Contributor

MabezDev commented Mar 5, 2024

So I added the source to .zprofile and it started working 🤷. This is odd because it's already in my .zshenv which I thought was always sourced, but I guess maybe not? Anyways, this change LGTM now.

@davidbarsky
Copy link
Contributor

i'll try this with a buck2-built sysroot once some issues at work are resolved.

@Veykril
Copy link
Member Author

Veykril commented Mar 6, 2024

Does this work?
@bors delegate=davidbarsky

@bors
Copy link
Contributor

bors commented Mar 6, 2024

✌️ @davidbarsky, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@Veykril
Copy link
Member Author

Veykril commented Mar 6, 2024

it does!

@davidbarsky
Copy link
Contributor

Sorry, am a little sick, so I was out of commission yesterday! A buck2-build sysroot is still successfully indexed with these changes. Thanks for waiting to let me check that.

@bors r=@Veykril

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit 6b48133 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 6, 2024

⌛ Testing commit 6b48133 with merge b85d38f...

@bors
Copy link
Contributor

bors commented Mar 6, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b85d38f to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 6, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b85d38f to master...

@bors bors merged commit b85d38f into rust-lang:master Mar 6, 2024
@bors
Copy link
Contributor

bors commented Mar 6, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust-analyzer does not call correct cargo binary
5 participants