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

Add rust-analyzer-proc-macro-srv binary, use it if found in sysroot #12858

Merged
merged 8 commits into from
Jul 26, 2022
Merged

Add rust-analyzer-proc-macro-srv binary, use it if found in sysroot #12858

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5f54ec0
Add a proc-macro-srv-cli crate
fasterthanlime Jul 23, 2022
fba6adf
Add doc comment to pass tidy check
fasterthanlime Jul 23, 2022
fd1b64e
Return result directly
fasterthanlime Jul 24, 2022
4364531
Name the binary 'proc-macro-srv'
fasterthanlime Jul 24, 2022
dadb832
Remember the difference between 'sysroot root' and 'sysroot src root'…
fasterthanlime Jul 25, 2022
74a2fad
Gate and rename binary, use it if it's in the sysroot
fasterthanlime Jul 25, 2022
6967751
Fix mix up in comments
fasterthanlime Jul 25, 2022
2c2520f
Allow specifying sysroot OR sysroot_src
fasterthanlime Jul 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/project-model/src/project_json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ use crate::cfg_flag::CfgFlag;
/// Roots and crates that compose this Rust project.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct ProjectJson {
/// e.g. `path/to/sysroot`
pub(crate) sysroot: Option<AbsPathBuf>,
/// e.g. `path/to/sysroot/lib/rustlib/src/rust`
pub(crate) sysroot_src: Option<AbsPathBuf>,
project_root: AbsPathBuf,
crates: Vec<Crate>,
Expand Down Expand Up @@ -52,6 +55,7 @@ impl ProjectJson {
/// configuration.
pub fn new(base: &AbsPath, data: ProjectJsonData) -> ProjectJson {
ProjectJson {
sysroot: data.sysroot.map(|it| base.join(it)),
sysroot_src: data.sysroot_src.map(|it| base.join(it)),
project_root: base.to_path_buf(),
crates: data
Expand Down Expand Up @@ -122,6 +126,7 @@ impl ProjectJson {

#[derive(Deserialize, Debug, Clone)]
pub struct ProjectJsonData {
sysroot: Option<PathBuf>,
sysroot_src: Option<PathBuf>,
crates: Vec<CrateData>,
}
Expand Down
21 changes: 16 additions & 5 deletions crates/project-model/src/sysroot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::{utf8_stdout, ManifestPath};
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct Sysroot {
root: AbsPathBuf,
src_root: AbsPathBuf,
crates: Arena<SysrootCrateData>,
}

Expand All @@ -35,6 +36,15 @@ impl ops::Index<SysrootCrate> for Sysroot {
}

impl Sysroot {
/// Returns sysroot directory, where `bin/`, `etc/`, `lib/`, `libexec/`
/// subfolder live, like:
/// `$HOME/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library`
pub fn src_root(&self) -> &AbsPath {
Copy link
Member

Choose a reason for hiding this comment

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

I think the names of this and root should be swapped. Also it did be nice if path/to/sysroot/lib/rustlib/src/rust could be derived from path/to/sysroot if the former is not given.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I got my comments mixed up, hang on

(I already did the name swap - thankfully ProjectJson already had the proper naming)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, comments fixed in 6967751

Also it did be nice if path/to/sysroot/lib/rustlib/src/rust could be derived from path/to/sysroot if the former is not given.

We never have a case where root is given but src_root is missing: previously, Sysroot could only be built when passing src_root. And simply joining lib/rustlib/src/rust is not a great idea. We already have a method for that (discover_sysroot_src_dir) and it respects RUST_SRC_PATH.

I considered making it so that building a Sysroot always takes root and an optional src_root (and if src_root is missing, we use the "discover" method), but decided against it, since:

  • There's a test case with a fake sysroot that only specifies src_root (it doesn't reproduce the directory structure of a real sysroot). We could fix that test case, no big deal, but...
  • We support rust-project.json, which up until now only allowed a sysroot_src field, not a sysroot field.

And going from sysroot_src to sysroot seems like a bad idea - it would involve checking that the last few path components are ["lib", "rustlib", "src", "rust], and removing them. I don't feel great about that.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should allow only giving the sysroot field and skipping sysroot_src.

And simply joining lib/rustlib/src/rust is not a great idea.

Why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not?

Because non-cargo projects (ProjectWorkspace::Json) might have "libstd sources" somewhere but not a "sysroot" like we expect for cargo projects, maybe? I'm not sure.

I think we should allow only giving the sysroot field and skipping sysroot_src.

The question is where?

For cargo projects, we already call discover_* functions for both root and src_root. Those respect RUST_SRC_PATH for example.

For ProjectWorkspace::Json projects, I'm not sure who sets the standard (maybe RA does?), but the rust-project.json format currently only has a sysroot_src field. Is that the case where you want to allow passing sysroot and "guess" sysroot_src ?

Copy link
Member

Choose a reason for hiding this comment

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

Because non-cargo projects (ProjectWorkspace::Json) might have "libstd sources" somewhere but not a "sysroot" like we expect for cargo projects, maybe? I'm not sure.

Using sysroot + lib/rustlib/src/rust as fallback for sysroot_src would work fine in that case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because non-cargo projects (ProjectWorkspace::Json) might have "libstd sources" somewhere but not a "sysroot" like we expect for cargo projects, maybe? I'm not sure.

Using sysroot + lib/rustlib/src/rust as fallback for sysroot_src would work fine in that case, right?

No, they specify the lib/rustlib/src/rust - we can't get what we need by joining additional path segments, we need to strip some, and that feels really iffy (but we can do it if we want)

There's zero codepaths where we have sysroot but not sysroot_src.

sysroot is a directory with src/, lib/, libexec/, etc/, etc. sysroot_src is a deeper path with libstd sources (sorry about the initial confusion, my comments were swapped).

Copy link
Member

Choose a reason for hiding this comment

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

You don't have sysroot_src if the user didn't specify one. I think the behavior should be:

if user specifies sysroot_src => use it, else use sysroot + lib/rustlib/src/rust

Or to put it another way:

sysroot_src is not specified by the user, set it to sysroot + lib/rustlib/src/rust

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't have sysroot_src if the user didn't specify one.

Ah, right. But right now no one is specifying sysroot either. That would require bazel (or other build systems that generate rust-project.json files) to set that field.

We should really discuss the Cargo / Json cases separately 😂

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 implemented what I think you and @jyn514 asked for in 2c2520f

&self.src_root
}

/// Returns sysroot "src" directory, where stdlib sources are located, like:
/// `$HOME/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu`
pub fn root(&self) -> &AbsPath {
&self.root
}
Expand All @@ -61,7 +71,7 @@ impl Sysroot {
tracing::debug!("Discovering sysroot for {}", dir.display());
let sysroot_dir = discover_sysroot_dir(dir)?;
let sysroot_src_dir = discover_sysroot_src_dir(&sysroot_dir, dir)?;
let res = Sysroot::load(sysroot_src_dir)?;
let res = Sysroot::load(sysroot_dir, sysroot_src_dir)?;
Ok(res)
}

Expand All @@ -71,14 +81,15 @@ impl Sysroot {
discover_sysroot_dir(current_dir).ok().and_then(|sysroot_dir| get_rustc_src(&sysroot_dir))
}

pub fn load(sysroot_src_dir: AbsPathBuf) -> Result<Sysroot> {
let mut sysroot = Sysroot { root: sysroot_src_dir, crates: Arena::default() };
pub fn load(sysroot_dir: AbsPathBuf, sysroot_src_dir: AbsPathBuf) -> Result<Sysroot> {
let mut sysroot =
Sysroot { root: sysroot_dir, src_root: sysroot_src_dir, crates: Arena::default() };

for path in SYSROOT_CRATES.trim().lines() {
let name = path.split('/').last().unwrap();
let root = [format!("{}/src/lib.rs", path), format!("lib{}/lib.rs", path)]
.into_iter()
.map(|it| sysroot.root.join(it))
.map(|it| sysroot.src_root.join(it))
.filter_map(|it| ManifestPath::try_from(it).ok())
.find(|it| fs::metadata(it).is_ok());

Expand Down Expand Up @@ -119,7 +130,7 @@ impl Sysroot {
};
anyhow::bail!(
"could not find libcore in sysroot path `{}`{}",
sysroot.root.as_path().display(),
sysroot.src_root.as_path().display(),
var_note,
);
}
Expand Down
7 changes: 5 additions & 2 deletions crates/project-model/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,11 @@ fn get_test_path(file: &str) -> PathBuf {

fn get_fake_sysroot() -> Sysroot {
let sysroot_path = get_test_path("fake-sysroot");
let sysroot_src_dir = AbsPathBuf::assert(sysroot_path);
Sysroot::load(sysroot_src_dir).unwrap()
// there's no `libexec/` directory with a `proc-macro-srv` binary in that
// fake sysroot, so we give them both the same path:
let sysroot_dir = AbsPathBuf::assert(sysroot_path);
let sysroot_src_dir = sysroot_dir.clone();
Sysroot::load(sysroot_dir, sysroot_src_dir).unwrap()
}

fn rooted_project_json(data: ProjectJsonData) -> ProjectJson {
Expand Down
12 changes: 9 additions & 3 deletions crates/project-model/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,14 @@ impl ProjectWorkspace {
project_json: ProjectJson,
target: Option<&str>,
) -> Result<ProjectWorkspace> {
let sysroot = match &project_json.sysroot_src {
Some(path) => Some(Sysroot::load(path.clone())?),
let sysroot = match project_json.sysroot_src.clone() {
Some(sysroot_src) => {
// if `sysroot` isn't specified (only `sysroot_src`), we won't have
// a real sysroot path, that's fine. it's just used to discover
// the standalone `proc-macro-srv` binary.
let sysroot = project_json.sysroot.clone().unwrap_or_else(|| sysroot_src.clone());
Some(Sysroot::load(sysroot, sysroot_src)?)
}
None => None,
};
let rustc_cfg = rustc_cfg::get(None, target);
Expand Down Expand Up @@ -345,7 +351,7 @@ impl ProjectWorkspace {
})
.chain(sysroot.iter().map(|sysroot| PackageRoot {
is_local: false,
include: vec![sysroot.root().to_path_buf()],
include: vec![sysroot.src_root().to_path_buf()],
exclude: Vec::new(),
}))
.chain(rustc.iter().flat_map(|rustc| {
Expand Down
30 changes: 28 additions & 2 deletions crates/rust-analyzer/src/reload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,34 @@ impl GlobalState {

if self.proc_macro_clients.is_empty() {
if let Some((path, args)) = self.config.proc_macro_srv() {
self.proc_macro_clients = (0..self.workspaces.len())
.map(|_| {
self.proc_macro_clients = self
.workspaces
.iter()
.map(|ws| {
let mut path = path.clone();
if let ProjectWorkspace::Cargo { sysroot, .. } = ws {
Copy link
Contributor

Choose a reason for hiding this comment

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

So at this point in time, we don't have any support for ProjectWorkspace::Json, even if sysroot is set?

Copy link
Member

@Veykril Veykril Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Did proc-macros ever work for project.json workspaces actually? From the looks of it we never even build build scripts for them since we can't really do that

Copy link
Member

Choose a reason for hiding this comment

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

Yes, proc macros should work with project.json workspaces. We have a field that allows specifying the path of the compiled proc macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me to where the proc-macro-srv is spawned for ProjectWorkspace::Json workspaces?

Copy link
Member

Choose a reason for hiding this comment

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

rust-analyzer/crates/project-model/src/workspace.rs

Line 484 in b1e9bcd

Some(it) => load_proc_macro(
I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@woody77 that match is definitely missing an arm I forgot to add when I added the additional field to rust-project.json's schema. I also didn't have any projects to test it with. I encourage you to open the small PR to fix it!

tracing::info!("Found a cargo workspace...");
if let Some(sysroot) = sysroot.as_ref() {
tracing::info!("Found a cargo workspace with a sysroot...");
let server_path = sysroot
.root()
.join("libexec")
.join("rust-analyzer-proc-macro-srv");
if std::fs::metadata(&server_path).is_ok() {
tracing::info!(
"And the server exists at {}",
server_path.display()
);
path = server_path;
} else {
tracing::info!(
"And the server does not exist at {}",
server_path.display()
);
}
}
}

ProcMacroServer::spawn(path.clone(), args.clone()).map_err(|err| {
let error = format!(
"Failed to run proc_macro_srv from path {}, error: {:?}",
Expand Down