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

scip: Allow customizing cargo config. #15633

Merged
merged 1 commit into from Sep 28, 2023
Merged

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Sep 19, 2023

Re-use the LSP config json for simplicity.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2023
@emilio
Copy link
Contributor Author

emilio commented Sep 19, 2023

r? @lnicola

This is useful for use-cases like mozsearch/mozsearch#652 (but also for any custom workspace shenanigans for which the rust-analyzer configuration is useful).

@lnicola
Copy link
Member

lnicola commented Sep 19, 2023

Regarding mozsearch/mozsearch#652, wouldn't setting CARGO_TARGET_DIR when running rust-analyzer scip work just as well?

also for any custom workspace shenanigans for which the rust-analyzer configuration is useful

Yeah, maybe that.


I'm just not sure we want to support this kind of configuration. But if we do, I think we want it as a global (top-level) option. It could also be useful for rust-analyzer diagnostics or analysis-stats, for example.

And it should probably be marked as unstable, since we I don't want to commit to this approach to configuration yet.

@emilio
Copy link
Contributor Author

emilio commented Sep 19, 2023

Regarding mozsearch/mozsearch#652, wouldn't setting CARGO_TARGET_DIR when running rust-analyzer scip work just as well?

No, it's already set a few lines before calling rust-analyzer scip, but...

also for any custom workspace shenanigans for which the rust-analyzer configuration is useful

Yeah, maybe that.

Yeah, in practice, eventually I want to use this for Firefox, which has a much complex workspace / vendoring set-up and requires also rust-analyzer.cargo.buildScripts.overrideCommand / rust-analyzer.check.overrideCommand.

I'm just not sure we want to support this kind of configuration. But if we do, I think we want it as a global option. It could also be useful for rust-analyzer diagnostics or analysis-stats, for example.

I think they could be useful but probably requires quite a bit more work to generalize this.

And it should probably be marked as unstable, since we I don't want to commit to this approach to configuration yet.

Yeah, making them unstable is fine with me. My understanding is that these are effectively already unstable tho? rust-analyzer --help says:

Subcommands and their flags do not provide any stability guarantees and may be removed or changed without notice. Top-level flags that are not are marked as [Unstable] provide backwards-compatibility and may be relied on.

I'm more than happy to adapt to changes in this area if the way of doing this changes in the future :)

@lnicola
Copy link
Member

lnicola commented Sep 19, 2023

All right, let's keep it only for skip for now. No need to mark it as unstable since it's not a top-level argument. But I'd still drop the cargo_config.sysroot part.

@bors delegate+

@bors
Copy link
Collaborator

bors commented Sep 19, 2023

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

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

@lnicola
Copy link
Member

lnicola commented Sep 19, 2023

No, it's already set a few lines before calling rust-analyzer scip, but...

Then it should already work AFAICT. But we only use rust-analyzer.server.extraEnv in the VS Code extension. You might want rust-analyzer.cargo.extraEnv instead.

@Veykril
Copy link
Member

Veykril commented Sep 19, 2023

This will most likely be superceded by rust-analyzer.toml once we have that

@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 Sep 20, 2023
Re-use the LSP config json for simplicity.
@emilio
Copy link
Contributor Author

emilio commented Sep 28, 2023

All right, let's keep it only for skip for now. No need to mark it as unstable since it's not a top-level argument. But I'd still drop the cargo_config.sysroot part.

Thanks! Rebased and removed that sysroot bit since it is indeed redundant. Just confirmed that using scip without config file still gets Discover as the sysroot strategy.

@bors r=lnicola

@bors
Copy link
Collaborator

bors commented Sep 28, 2023

📌 Commit 791e6c8 has been approved by lnicola

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 28, 2023

⌛ Testing commit 791e6c8 with merge f19479a...

@bors
Copy link
Collaborator

bors commented Sep 28, 2023

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing f19479a to master...

@bors bors merged commit f19479a into rust-lang:master Sep 28, 2023
10 checks passed
@emilio emilio deleted the scip-cargo-config branch September 28, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants