Skip to content

minor: Use CargoConfig.extra_args for fetching metadata #14642

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
Merged
merged 2 commits into from
Apr 25, 2023
Merged

minor: Use CargoConfig.extra_args for fetching metadata #14642

merged 2 commits into from
Apr 25, 2023

Conversation

jakhh8
Copy link
Contributor

@jakhh8 jakhh8 commented Apr 23, 2023

Fixes #14510

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2023
Veykril
Veykril reviewed Apr 24, 2023
View reviewed changes
crates/project-model/src/cargo_workspace.rs Outdated
@@ -297,6 +297,7 @@ impl CargoWorkspace {
let other_options: Vec<_> = targets
.into_iter()
.flat_map(|target| ["--filter-platform".to_string(), target])
.chain(config.extra_args.clone())
Copy link
Member

Choose a reason for hiding this comment

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

This is only set when the targets are empty here, we should put this after the !targets.is_empty() branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now

@Veykril
Copy link
Member

Veykril commented Apr 25, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2023

📌 Commit 62c9d96 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 25, 2023

⌛ Testing commit 62c9d96 with merge 943d2a8...

@bors
Copy link
Contributor

bors commented Apr 25, 2023

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

@bors bors merged commit 943d2a8 into rust-lang:master Apr 25, 2023
@utkarshgupta137 utkarshgupta137 mentioned this pull request May 1, 2023
Closed
@utkarshgupta137
Copy link

This change broke a workaround many of us have been using to have a separate directory for rust-analyzer: #6007 (comment).
I don't think this change is a good idea because it limits the arguments in cargo.extraArgs to only those which are compatible with cargo-metadata. cargo-metadata supports a small subsection of the arguments supported by other commands such as cargo-check. In particular, I think the following arguments could be useful, which are no longer allowed: package, exclude, jobs, profile, target-dir, release.
If we want to keep this, then at least the incompatible arguments should be removed from cargo-metadata, or a brute-force fallback without extraArgs could also work. I would also note that the OP of the issue was able to workaround his problem using a .cargo/config file.

@Veykril
Copy link
Member

Veykril commented May 2, 2023

#14712 should patch over this. Unstable flags seem to be the only relevant ones so thats all we pass through then

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

@Veykril Veykril Veykril left review comments

Assignees
No one assigned
Labels
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.

rust-analyzer.cargo.extraArgs not working
5 participants
@jakhh8 @Veykril @bors @utkarshgupta137 @rustbot