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.

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

Collapse invocationLocation and invocationStrategy into a single setting #17848

Closed
RalfJung opened this issue Aug 10, 2024 · 3 comments · Fixed by #17888
Closed

Collapse invocationLocation and invocationStrategy into a single setting #17848

RalfJung opened this issue Aug 10, 2024 · 3 comments · Fixed by #17888
Assignees
Labels
A-config configuration E-easy E-has-instructions Issue has some instructions and pointers to code to get started

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 10, 2024

These options were added to explore how RA can best work with repos like rustc and Miri that need special non-cargo invocations (./x.py ..././miri ...). The consensus that generally has emerged is that these scripts should be invoked exactly once, in the root, and they will emit all the diagnostics for all parts of the project with paths relative to the root. There were ideas of having RA pass --manifest-path or so, with template magic available in overrideCommand to control where to put that flag; I don't think any more that those are promising.

If there are no other usecases being covered by these flags then most of the freedom provided by having two independent settings, invocationLocation and invocationStrategy, isn't actually needed. A possible simplification here might be to only have invocationLocation, and if it is set to "root" then that also implies running it only once (since it seems kind of pointless to run the same command in the same location multiple times). invocationStrategy could then be removed. Or conversely, invocationStrategy is preserved and setting it to once means "run it once, in the root".

@Veykril
Copy link
Member

Veykril commented Aug 10, 2024

+1 for keeping invocationStrategy, I think that is the clearer name one to imply both things (though im also open to a new name for this altogether).

To add context, these settings were generally never meant to stick around, they were only an escape hatch for rustc's x tool, so simplifying this is definitely wanted. We still have the --saved-file stuff, but those also mainly want the root-once behavior to my knowledge so they aren't relevant to this.

@Veykril Veykril added E-easy E-has-instructions Issue has some instructions and pointers to code to get started labels Aug 10, 2024
@Veykril
Copy link
Member

Veykril commented Aug 10, 2024

So the best way to go about this is to remove the one config half here

/// Specifies the working directory for running build scripts.
/// - "workspace": run build scripts for a workspace in the workspace's root directory.
/// This is incompatible with `#rust-analyzer.cargo.buildScripts.invocationStrategy#` set to `once`.
/// - "root": run build scripts in the project's root directory.
/// This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
/// is set.
cargo_buildScripts_invocationLocation: InvocationLocation = InvocationLocation::Workspace,
/// Specifies the invocation strategy to use when running the build scripts command.
/// If `per_workspace` is set, the command will be executed for each workspace.
/// If `once` is set, the command will be executed once.
/// This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
/// is set.
cargo_buildScripts_invocationStrategy: InvocationStrategy = InvocationStrategy::PerWorkspace,
and here
/// Specifies the working directory for running checks.
/// - "workspace": run checks for workspaces in the corresponding workspaces' root directories.
// FIXME: Ideally we would support this in some way
/// This falls back to "root" if `#rust-analyzer.check.invocationStrategy#` is set to `once`.
/// - "root": run checks in the project's root directory.
/// This config only has an effect when `#rust-analyzer.check.overrideCommand#`
/// is set.
check_invocationLocation | checkOnSave_invocationLocation: InvocationLocation = InvocationLocation::Workspace,
/// Specifies the invocation strategy to use when running the check command.
/// If `per_workspace` is set, the command will be executed for each workspace.
/// If `once` is set, the command will be executed once.
/// This config only has an effect when `#rust-analyzer.check.overrideCommand#`
/// is set.
check_invocationStrategy | checkOnSave_invocationStrategy: InvocationStrategy = InvocationStrategy::PerWorkspace,
and let the compiler guide you in removing whatever depends on that (such a great thing with rustc really).

In the build scripts part changes are simply, the branch here can be dropped for picking workspace.root() unconditionally

let current_dir = match &config.invocation_location {
InvocationLocation::Root(root) if config.run_build_script_command.is_some() => root,
_ => workspace.workspace_root(),
};

and the branch here can be dropped, we will always use root here
let current_dir = match &config.invocation_location {
InvocationLocation::Root(root) => root,
InvocationLocation::Workspace => {
return Err(io::Error::new(
io::ErrorKind::Other,
"Cannot run build scripts from workspace with invocation strategy `once`",
))
}
};

For flycheck you'll have to drop the outer match here https://github.com/veykril/rust-analyzer/blob/56f63dfd8aeebf80e3fc87894fa3d5a40f98a329/crates/rust-analyzer/src/flycheck.rs#L434-L449 and change the InvocationStrategy::Once arm to use the root, so that means we'll have to thread the root through InvocationStrategy::Once (change it to what Invocationlocation::Root was, holding a path).

@Tyrubias
Copy link
Contributor

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-config configuration E-easy E-has-instructions Issue has some instructions and pointers to code to get started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants