-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Extend TargetSpec
functionality to rust-project.json
#16135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ use rustc_hash::FxHashMap; | |
use serde::{de, Deserialize}; | ||
use span::Edition; | ||
|
||
use crate::cfg_flag::CfgFlag; | ||
use crate::{cfg_flag::CfgFlag, TargetKind}; | ||
|
||
/// Roots and crates that compose this Rust project. | ||
#[derive(Clone, Debug, Eq, PartialEq)] | ||
|
@@ -73,20 +73,37 @@ pub struct ProjectJson { | |
/// useful in creating the crate graph. | ||
#[derive(Clone, Debug, Eq, PartialEq)] | ||
pub struct Crate { | ||
pub(crate) display_name: Option<CrateDisplayName>, | ||
pub(crate) root_module: AbsPathBuf, | ||
pub(crate) edition: Edition, | ||
pub(crate) version: Option<String>, | ||
pub(crate) deps: Vec<Dependency>, | ||
pub(crate) cfg: Vec<CfgFlag>, | ||
pub(crate) target: Option<String>, | ||
pub(crate) env: FxHashMap<String, String>, | ||
pub(crate) proc_macro_dylib_path: Option<AbsPathBuf>, | ||
pub(crate) is_workspace_member: bool, | ||
pub(crate) include: Vec<AbsPathBuf>, | ||
pub(crate) exclude: Vec<AbsPathBuf>, | ||
pub(crate) is_proc_macro: bool, | ||
pub(crate) repository: Option<String>, | ||
pub display_name: Option<CrateDisplayName>, | ||
pub root_module: AbsPathBuf, | ||
pub edition: Edition, | ||
pub version: Option<String>, | ||
pub deps: Vec<Dependency>, | ||
pub cfg: Vec<CfgFlag>, | ||
pub target: Option<String>, | ||
pub env: FxHashMap<String, String>, | ||
pub proc_macro_dylib_path: Option<AbsPathBuf>, | ||
pub is_workspace_member: bool, | ||
pub include: Vec<AbsPathBuf>, | ||
pub exclude: Vec<AbsPathBuf>, | ||
pub is_proc_macro: bool, | ||
pub repository: Option<String>, | ||
pub target_spec: Option<TargetSpec>, | ||
} | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq)] | ||
pub struct TargetSpec { | ||
pub manifest_file: AbsPathBuf, | ||
pub target_label: String, | ||
pub target_kind: TargetKind, | ||
pub runnables: Runnables, | ||
pub flycheck_command: Vec<String>, | ||
} | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq)] | ||
pub struct Runnables { | ||
pub check: Vec<String>, | ||
pub run: Vec<String>, | ||
pub test: Vec<String>, | ||
} | ||
|
||
impl ProjectJson { | ||
|
@@ -121,6 +138,20 @@ impl ProjectJson { | |
None => (vec![root_module.parent().unwrap().to_path_buf()], Vec::new()), | ||
}; | ||
|
||
let target_spec = match crate_data.target_spec { | ||
Some(spec) => { | ||
let spec = TargetSpec { | ||
manifest_file: absolutize_on_base(spec.manifest_file), | ||
target_label: spec.target_label, | ||
target_kind: spec.target_kind.into(), | ||
runnables: spec.runnables.into(), | ||
flycheck_command: spec.flycheck_command, | ||
}; | ||
Some(spec) | ||
} | ||
None => None, | ||
}; | ||
|
||
Crate { | ||
display_name: crate_data | ||
.display_name | ||
|
@@ -149,6 +180,7 @@ impl ProjectJson { | |
exclude, | ||
is_proc_macro: crate_data.is_proc_macro, | ||
repository: crate_data.repository, | ||
target_spec, | ||
} | ||
}) | ||
.collect(), | ||
|
@@ -172,6 +204,14 @@ impl ProjectJson { | |
pub fn path(&self) -> &AbsPath { | ||
&self.project_root | ||
} | ||
|
||
pub fn crate_by_root(&self, root: &AbsPath) -> Option<Crate> { | ||
self.crates | ||
.iter() | ||
.filter(|krate| krate.is_workspace_member) | ||
.find(|krate| &krate.root_module == root) | ||
.cloned() | ||
} | ||
} | ||
|
||
#[derive(Deserialize, Debug, Clone)] | ||
|
@@ -201,6 +241,8 @@ struct CrateData { | |
is_proc_macro: bool, | ||
#[serde(default)] | ||
repository: Option<String>, | ||
#[serde(default)] | ||
target_spec: Option<TargetSpecData>, | ||
} | ||
|
||
#[derive(Deserialize, Debug, Clone)] | ||
|
@@ -216,6 +258,55 @@ enum EditionData { | |
Edition2024, | ||
} | ||
|
||
#[derive(Deserialize, Debug, Clone)] | ||
pub struct TargetSpecData { | ||
manifest_file: Utf8PathBuf, | ||
target_label: String, | ||
target_kind: TargetKindData, | ||
runnables: RunnablesData, | ||
flycheck_command: Vec<String>, | ||
} | ||
|
||
#[derive(Deserialize, Debug, Clone)] | ||
pub struct RunnablesData { | ||
check: Vec<String>, | ||
run: Vec<String>, | ||
test: Vec<String>, | ||
} | ||
|
||
#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Deserialize)] | ||
#[serde(rename_all = "camelCase")] | ||
pub enum TargetKindData { | ||
Bin, | ||
/// Any kind of Cargo lib crate-type (dylib, rlib, proc-macro, ...). | ||
Lib, | ||
Example, | ||
Test, | ||
Bench, | ||
BuildScript, | ||
Other, | ||
} | ||
Comment on lines
+279
to
+288
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ties the cargo workspace architecture into project-json, I'm not sure that this was something @matklad had in mind when giving this alternative format for project descriptions. Is there a need for things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think #16135 (comment) answers this, but, yeah good catch of the specific smell here! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're not really needed, no—they were meant to try and make |
||
|
||
impl From<TargetKindData> for TargetKind { | ||
fn from(value: TargetKindData) -> Self { | ||
match value { | ||
TargetKindData::Bin => TargetKind::Bin, | ||
TargetKindData::Lib => TargetKind::Lib { is_proc_macro: false }, | ||
TargetKindData::Example => TargetKind::Example, | ||
TargetKindData::Test => TargetKind::Test, | ||
TargetKindData::Bench => TargetKind::Bench, | ||
TargetKindData::BuildScript => TargetKind::BuildScript, | ||
TargetKindData::Other => TargetKind::Other, | ||
} | ||
} | ||
} | ||
|
||
impl From<RunnablesData> for Runnables { | ||
fn from(value: RunnablesData) -> Self { | ||
Runnables { check: value.check, run: value.run, test: value.test } | ||
} | ||
} | ||
|
||
impl From<EditionData> for Edition { | ||
fn from(data: EditionData) -> Self { | ||
match data { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes on JSON side of things here:
I'd maybe call the top-level thing
build_info
, as thre might be multiple targets (in buck2 sense) concerning a single crate. And, well, target is already overloaded, so it's better to avoid pilling more meanings onto that poor word.I think the core feature here should be an inclusion of opaque blob of data, which is transmitted over LSP so that the custom client may interpret this. So, I'd say the basis should look like
Eg, one can imagine a situation where all builds are remote, and the way you "run" software is by issueing an HTTP request to a remote execution server, and opening a page in the web-browser with results. In this situation, the
data
would be an URL to call, and the client-side extension would take care of HTTPing.But we also should support "standard" workflows where you just run some command line.
So, something like (pardon my TypeScript)
This shouldn't use a json map, but rather an array, to be more flexible. Eg, in the future we might want to add
--release
and non---release
versions of tests, and with a list it should be simpler. More generally, I've recently read some JSON API guidlines somewhere (don't remember whether though), and one guideline that stuck with me is that you should represent maps in JSON as arrays (assoc lists). I think that's a good guideline and something we should be using in gerneral.Then, the
kind
should be a semi-open enum. rust-analyzer should treatbuild
,run
,check
andtest
specially, but it should also be able to show other tasks in GUI. Eg, you can imagine a "flush" task for an embedded build system, or some such.We might want to split
check
into two --- one check that is run for the user and displays errors in the terminal, and another check that runs with message-format=json and is used for flycheck. I guess we could call thosecheck
andflycheck
?For tests, we need to think how to enable filtering. The core feature is that you can run a single test using
cargo test -- $TEST_NAME
, and all tests usingcargo test
. Note that this might be very different in non-cargo build systems. Eg, it could bebuilder test
andbuilder test --test-filter=$TEST_NAME
, wherebuilder test --test-filter=''
would run zero tests, rather than all tests. So I think for test, we should also split it into two: "testall" and "testone". And, for "testone", I think we want to makeargv
a template. That is, argv could look likeWe shouldn't assume that filter is always last, and that its always a separate argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback here!
That's a fair point. I didn't intend for the naming to be Buck/Bazel-esque "targets", I mean to invoke the target concept in Cargo, of all things. In any case,
target
is too overloaded and I'll move over tobuild_info
.Is the idea that a companion extension to
rust-lang.rust-analyzer
(e.g., something for Buck or Bazel) would be responsible for assembling that opaque blob of data into a some sort of meaningful runnable command or is more that since I'm making this change now, I should make it more futureproof ("[B]ut we also should support "standard" workflows where you just run some command line.")? I ask since I'm working on a variable interpreter/template thingy, which—I think—should make expressing arbitrary build/test commands a lot easier in rust-analyzer itself, which reduces the pressure/need for end-users to install other extensions for runnables.That seems reasonable to me! I was considering how to tackle this for Buck1 and this seems like a good approach.
Yeah, that seems smart.
I think now's a good time to make this change, especially if the new runnable API is a breaking change.
For tests, we need to think how to enable filtering. The core feature is that you can run a single test using
cargo test -- $TEST_NAME
, and all tests usingcargo test
. Note that this might be very different in non-cargo build systems. Eg, it could bebuilder test
andbuilder test --test-filter=$TEST_NAME
, wherebuilder test --test-filter=''
would run zero tests, rather than all tests. So I think for test, we should also split it into two:"testall"
and"testone"
. And, for"testone"
, I think we want to makeargv
a template. That is, argv could look like: [redacted for quoting ease]I don't think I've fully gotten this into my head yet (sorry, lots of travel!) but I think I see the utility of a change like this.
I think I'd like like a
debug
variant as well, but I haven't really through this specific aspect through.Footnotes
For context, buck2 is moving to a modifier-style approach. A user can write
buck2 build repo//foo:bar?linux,release
, which translate to "build repo//foo:bar
for Linux usingrelease
". there's a more verbose--modifier
-style API, but I suspect that both options can work when called from rust-analyzer or I suspect the Buck team will be flexible in implementing this. ↩