Skip to content
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

feat: Add rust-analyzer.cargo.allTargets to configure passing --all-targets to cargo invocations #16924

Merged
merged 1 commit into from Apr 1, 2024
Merged

feat: Add rust-analyzer.cargo.allTargets to configure passing --all-targets to cargo invocations #16924

merged 1 commit into from Apr 1, 2024

Conversation

poliorcetics
Copy link
Contributor

Closes #16859

Unresolved question:

Should this be a setting for build scripts only ? All the other --all-targets I found where already covered by checkOnSave.allTargets

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2024
@Veykril Veykril mentioned this pull request Mar 25, 2024
Closed
@jakzale
Copy link

jakzale commented Mar 25, 2024

Closes #16859

Unresolved question:

Should this be a setting for build scripts only ? All the other --all-targets I found where already covered by checkOnSave.allTargets

When working with targets that

  • are building their own std, and
  • are being built with restricted_std,

passing --all-targets will break both cargo build and cargo check, as cargo will try to build the test crate, which is not marked with restricted_std.

See the PR referenced above.

@Veykril
Copy link
Member

Veykril commented Mar 25, 2024

Should this be a setting for build scripts only ? All the other --all-targets I found where already covered by checkOnSave.allTargets

Yes, all other invocations should be check related so thats fine. What we should do is have the check setting inherit the cargo setting here if its unset. Thats how all the other ones work for this pairing

@poliorcetics
Copy link
Contributor Author

Yes, all other invocations should be check related so thats fine. What we should do is have the check setting inherit the cargo setting here if its unset. Thats how all the other ones work for this pairing

So the checkOnSave.allTargets setting would change from bool to Option<bool> and become None by default ?

@Veykril
Copy link
Member

Veykril commented Apr 1, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Apr 1, 2024

📌 Commit 174af88 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 1, 2024

⌛ Testing commit 174af88 with merge 2678660...

@bors
Copy link
Collaborator

bors commented Apr 1, 2024

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

@bors bors merged commit 2678660 into rust-lang:master Apr 1, 2024
11 checks passed
RalfJung
RalfJung reviewed Apr 1, 2024
View reviewed changes
editors/code/package.json
@@ -546,6 +546,11 @@
"minimum": 0,
"maximum": 255
},
"rust-analyzer.cargo.allTargets": {
"markdownDescription": "Pass `--all-targets` to cargo invocation. Overridden by `#rust-analyzer.check.allTargets#`\nwhen the latter is set.",
Copy link
Member

Choose a reason for hiding this comment

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

Is it truly overridden? It seems to me like it is only partially overridden. Specifically, the build script / proc macro build will always use the value of rust-analyzer.cargo.allTargets even if rust-analyzer.check.allTargets is set.

If that's correct then the docs are wrong or at least misleading.

Copy link
Member

@Veykril Veykril Apr 1, 2024 •
edited

Choose a reason for hiding this comment

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

Indeed, I misread this. The docs are wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have precised "for check calls" maybe

Copy link
Member

Choose a reason for hiding this comment

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

I adjusted it in #16988 making it the same as the docs for allFeatures

RalfJung
RalfJung reviewed Apr 1, 2024
View reviewed changes
crates/rust-analyzer/src/config.rs
@@ -1273,6 +1276,7 @@ impl Config {
let sysroot_query_metadata = self.data.cargo_sysrootQueryMetadata;

CargoConfig {
all_targets: self.data.cargo_allTargets,
Copy link
Member

Choose a reason for hiding this comment

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

If check_allTargets is intended to overwrite cargo_allTargets, then that would have to be done here as well.

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

@RalfJung RalfJung RalfJung left review comments

@Veykril Veykril Veykril approved these changes

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.

Build script build is run with --all-targets even if rust-analyzer.check.allTargets is false
6 participants
@poliorcetics @jakzale @Veykril @bors @RalfJung @rustbot