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

Add config option to use rust-analyzer specific target dir #15681

Merged
merged 7 commits into from Oct 9, 2023
Merged

Add config option to use rust-analyzer specific target dir #15681

merged 7 commits into from Oct 9, 2023

Conversation

Tyrubias
Copy link
Contributor

@Tyrubias Tyrubias commented Sep 29, 2023 •
edited

Adds a Rust Analyzer configuration option to set a custom target directory for builds. This is a workaround for Rust Analyzer blocking debug builds while running cargo check. This change should close #6007.

This is my first time contributing to this project, so any feedback regarding best practices that I'm not aware of are greatly appreciated! Thanks to all the maintainers for their hard work on this project and reviewing contributions.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2023
Veykril
Veykril requested changes Sep 29, 2023
View reviewed changes
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
@Tyrubias
Copy link
Contributor Author

Tyrubias commented Oct 1, 2023

@Veykril I realized it might not make sense for us to pass --target-dir to a Flycheck custom command. Should we set CARGO_TARGET_DIR for Flycheck custom commands?

@Tyrubias Tyrubias requested a review from Veykril October 1, 2023 02:23
@Veykril
Copy link
Member

Veykril commented Oct 1, 2023

Why would the env var make more sense then passing the flag?

@Tyrubias
Copy link
Contributor Author

Tyrubias commented Oct 1, 2023 •
edited

Is it not possible for the Flycheck custom command to invoke another command that invokes cargo? In that case adding --target-dir would cause an error, but then the environment variable would eventually be inherited by the cargo process. Please let me know if I'm mistaken.

@Veykril
Copy link
Member

Veykril commented Oct 4, 2023 •
edited

That's fine, if you use a command override most of the settings no longer apply. In general we expect people to completely build up their command if they use an override. And if your custom command invokes cargo later it should be your commands job to pass the target dir options.

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2023
@Tyrubias
Copy link
Contributor Author

Tyrubias commented Oct 4, 2023 •
edited

That makes sense, I've changed it back to using --target-dir instead of CARGO_TARGET_DIR. Are there any more tests I should add for this behavior? Thanks.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2023
Veykril
Veykril reviewed Oct 9, 2023
View reviewed changes
crates/flycheck/src/lib.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2023
@Tyrubias
Add config option to use rust-analyzer specific target dir
aeef7b6 
Adds a Rust Analyzer configuration option to set a custom
target directory for builds. This is a workaround for Rust Analyzer
blocking debug builds while running `cargo check`. This change
should close rust-lang#6007
@Tyrubias
Add dedicated target_dir field to CargoConfig and FlycheckConfig
53b6700 
Add dedicated field for `target_dir` in the configurations for Cargo
and Flycheck. Also change the directory to be a `PathBuf` as opposed to
a `String` to be more appropriate to the operating system.
@Tyrubias
Clarify documentation on new parameter
ef0b3bb
@Tyrubias
Set CARGO_TARGET_DIR when using Flycheck custom command
3682c37
@Tyrubias
Pass target directory as flag instead of env variable
2290cc3
@Tyrubias
Don't pass any target directory options to custom flycheck
9771e1e
@Tyrubias
Addressed PR style comments
a39d207
@Tyrubias
Copy link
Contributor Author

Tyrubias commented Oct 9, 2023

Thank you so much for the mentorship! I've addressed your comments.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 9, 2023
@Veykril
Copy link
Member

Veykril commented Oct 9, 2023

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Oct 9, 2023

📌 Commit a39d207 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 9, 2023

⌛ Testing commit a39d207 with merge d646ae8...

@bors
Copy link
Collaborator

bors commented Oct 9, 2023

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

@bors bors merged commit d646ae8 into rust-lang:master Oct 9, 2023
10 checks passed
@Tyrubias Tyrubias deleted the custom_target_dir branch October 9, 2023 07:53
lnicola
lnicola reviewed Oct 16, 2023
View reviewed changes
crates/rust-analyzer/src/config.rs
///
/// Set to `true` to use a subdirectory of the existing target directory or
/// set to a path relative to the workspace to use that path.
rust_analyzerTargetDir: Option<TargetDirectory> = "null",
Copy link
Member

Choose a reason for hiding this comment

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

Would have made sense to call this cargo_ instead.

@matklad
Copy link
Member

matklad commented Oct 16, 2023 •
edited

Brilliant idea!

EDIT: this was meant for #15713 :-)

The idea in this PR is more obvious, but, really, this is a big feature we should have implemnted years ago, so kudos here as well ❤️

@alice-i-cecile alice-i-cecile mentioned this pull request Oct 26, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lnicola lnicola lnicola 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.

Feature request: Option to use rust-analyzer-specific target directory (relative to target directory)
6 participants
@Tyrubias @Veykril @bors @matklad @lnicola @rustbot