Allow rust-project.json to specify sysroot workspace #19096

darichey · 2025-02-05T00:28:02Z

Following up on #18590 (comment) and #18788 to add a sysroot_project key to rust-project.json which allows specifying a nested rust-project for the sysroot. This gives control over the sysroot layout to the rust-project generator (e.g., Buck). At work, we can build the sysroot with Buck, so this change allows us to make use of that instead of the stitched sysroot. Most users of Buck and Bazel will still want to use Cargo for the sysroot.

darichey · 2025-02-05T00:30:44Z

There is still some work to do here, but I wanted to get feedback on the overall approach before moving forward and making the corresponding changes in Buck's rust-project.

We can also delete stitched sysroot here if we change Cargo to be the fallback.

Veykril

Seems reasonable to me, I assume (should we rid of the stitched variant), setting sysroot, sysroot_src and sysroot_project as None would imply not to load a sysroot? Or should we have a separate flag to opt-into not loading. Maybe just sysroot_src and sysroot_project being None should opt-out (as a sysroot dir is still useful for tools). I don't know the use-cases of alternate build systems too well here

Veykril · 2025-02-05T07:44:59Z

crates/project-model/src/workspace.rs

@@ -566,7 +573,9 @@ impl ProjectWorkspace {
                        Some(PackageRoot { is_local: false, include, exclude })
                    })
                    .collect(),
-                SysrootWorkspace::Stitched(_) | SysrootWorkspace::Empty => vec![],
+                SysrootWorkspace::Json(_)


Do we not need to handle this as well?

Veykril · 2025-02-05T07:49:32Z

crates/project-model/src/project_json.rs

@@ -65,6 +65,7 @@ pub struct ProjectJson {
    pub(crate) sysroot: Option<AbsPathBuf>,
    /// e.g. `path/to/sysroot/lib/rustlib/src/rust/library`
    pub(crate) sysroot_src: Option<AbsPathBuf>,
+    pub(crate) sysroot_project: Option<ProjectJsonData>,


Shouldn't this be Option<ProjectJson>?

Veykril · 2025-02-05T07:50:34Z

crates/project-model/src/project_json.rs

@@ -65,6 +65,7 @@ pub struct ProjectJson {
    pub(crate) sysroot: Option<AbsPathBuf>,
    /// e.g. `path/to/sysroot/lib/rustlib/src/rust/library`
    pub(crate) sysroot_src: Option<AbsPathBuf>,
+    pub(crate) sysroot_project: Option<ProjectJsonData>,


Comment above would be nice

darichey · 2025-02-06T16:34:23Z

I've added another commit that deletes stitched sysroot. The current behavior is...

For cargo projects...
1. Try cargo
2. If cargo fails, no sysroot is loaded
For json projects...
1. If we have both sysroot_src and sysroot_project, use that
2. Otherwise, try cargo
3. If cargo fails, no sysroot is loaded

I'm not aware of a use case for explicitly opting out of the sysroot entirely. Is there a way to do that for cargo projects? If not, I don't think we need it for json projects either.

Veykril

Will need a rebase over my latest changes which likely conflict a fair bit, sorry. I don't think cargo projects ever want to specify not having a sysroot rust source so removing that should be fine. I do now wonder though whether rust-project.json even wants a sysroot source field at all, or if instead we want to expose the lang origin thing for its specified crate data. That is have it embed the sysroot into its crate graph itself and tag the relevant crates accordingly?

Veykril · 2025-02-14T12:55:44Z

@davidbarsky regarding my comment there, specifically

That is have it embed the sysroot into its crate graph itself and tag the relevant crates accordingly?

I believe that is something you've been thinking about this week basically (in the space of cargo metadata, but here it applies more directly wrt to the json format)

davidbarsky · 2025-02-14T16:27:59Z

@davidbarsky regarding my comment there, specifically

That is have it embed the sysroot into its crate graph itself and tag the relevant crates accordingly?

I believe that is something you've been thinking about this week basically (in the space of cargo metadata, but here it applies more directly wrt to the json format)

Yeah, I have been thinking about this because I was trying to see if there's a way to unify the paths for handling a rust-project.json with a rustup-managed rustc source, a rust-project.json with a build system-managed rustc source, cargo-metadata as it exists today, and cargo-metadata with a -Zbuild-std. This was mostly a dead-end because it doesn't seem feasible to do that today, but directly tagging crates in the crate graph with an attribute along the lines of "this is a rust-src/prelude crate" is one possible direction for a future design of rust-project.json. Regardless, I think that the approach @darichey took here is probably the most optimal under the current circumstances.

I also chatted with @darichey and @capickett this week, where I learned that Buck can use a Buckified rust-src (this component isn't open-source, but it should be!) where the overall build looks extremely similar to where -Zbuild-std will probably end up: namely, there's only one cargo-metadata-shaped call, so the rust-src crates are included inline. However, I imagine most non-Facebook/Google users of Buck and Bazel would prefer to use a rustup-managed sysroot because they don't want to touch RUST_BOOTSTRAP=1.

Veykril · 2025-02-14T16:32:07Z

crates/project-model/src/sysroot.rs

+                    SysrootWorkspace::Json(project_json) => project_json
+                        .crates()
+                        .filter_map(|(_, krate)| krate.display_name.clone())
+                        .any(|name| name.canonical_name().as_str() == "core-0.0.0"), // FIXME: this is buck-specific and should be handled there instead


A bit iffy about this, we might want to disable the check for json sysroots for now?

davidbarsky · 2025-02-14T17:06:40Z

I just realized that I actually don't know what a "stitched workspace" refers to: I can't really find it in my limited scrollback!

Summary: rust-analyzer wants to drop support for their "stitched sysroot" (where it will assume the structure of the sysroot) in favor of relying on the build tool to tell it the sysroot layout. Since we have a buckified sysroot, that can be buck, and we can include a nested JsonProject for the sysroot. Corresponding rust-analyzer change: rust-lang/rust-analyzer#19096 Reviewed By: davidbarsky Differential Revision: D69328620 fbshipit-source-id: ee0b461b2e144bfefd30970d6f6ce35ae89e8ee1

Veykril · 2025-02-25T17:46:54Z

I just realized that I actually don't know what a "stitched workspace" refers to: I can't really find it in my limited scrollback!

Oh I thought I replied to this by mail, sorry. Stitched just means we manually picked the crates in the sysroot from a known list. That logic predates the sysroot now shipping a proper cargo workspace (which is why we can drop support for it now, its been long enough)

darichey · 2025-02-26T17:59:24Z

Corresponding buck2 changes:

I've tested this with those changes and the sysroot loads properly.

darichey · 2025-02-26T19:02:42Z

crates/project-model/src/tests.rs

@@ -225,18 +223,6 @@ fn rust_project_cfg_groups() {
    check_crate_graph(crate_graph, expect_file!["../test_data/output/rust_project_cfg_groups.txt"]);
 }

-#[test]


I deleted this test because it's really testing a behavior of the stitched sysroot.

Veykril · 2025-02-27T11:22:49Z

Thanks!

rustbot added the S-waiting-on-review label Feb 5, 2025

Veykril reviewed Feb 5, 2025

View reviewed changes

darichey force-pushed the rust-project-sysroot branch from 053f2bd to 597b00d Compare February 6, 2025 16:29

Veykril reviewed Feb 14, 2025

View reviewed changes

Veykril approved these changes Feb 14, 2025

View reviewed changes

darichey force-pushed the rust-project-sysroot branch from 597b00d to b5a4350 Compare February 25, 2025 17:39

darichey force-pushed the rust-project-sysroot branch from b5a4350 to 16024dd Compare February 26, 2025 17:56

darichey marked this pull request as ready for review February 26, 2025 17:57

darichey force-pushed the rust-project-sysroot branch from 16024dd to 0a456cf Compare February 26, 2025 19:01

darichey commented Feb 26, 2025

View reviewed changes

darichey added 2 commits February 26, 2025 14:18

Allow rust-project.json to specify sysroot workspace

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

ca5a35e

Drop support for stitched sysroot

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

18a678e

darichey force-pushed the rust-project-sysroot branch from 0a456cf to 18a678e Compare February 26, 2025 20:19

Veykril approved these changes Feb 27, 2025

View reviewed changes

Veykril added this pull request to the merge queue Feb 27, 2025

Veykril mentioned this pull request Feb 27, 2025

Fix sysroot crate-graph construction not mapping crate-ids for proc-macros #19241

Merged

Merged via the queue into rust-lang:master with commit e50bc18 Feb 27, 2025
9 checks passed

Veykril mentioned this pull request Mar 3, 2025

Bump minimum supported toolchain to 1.82 #19275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow rust-project.json to specify sysroot workspace #19096

Allow rust-project.json to specify sysroot workspace #19096

darichey commented Feb 5, 2025

darichey commented Feb 5, 2025

Veykril left a comment

Veykril Feb 5, 2025

Veykril Feb 5, 2025

Veykril Feb 5, 2025

darichey commented Feb 6, 2025 •

edited

Loading

Veykril left a comment

Veykril commented Feb 14, 2025

davidbarsky commented Feb 14, 2025

Veykril Feb 14, 2025

davidbarsky commented Feb 14, 2025

Veykril commented Feb 25, 2025

darichey commented Feb 26, 2025

darichey Feb 26, 2025

Veykril commented Feb 27, 2025

Allow rust-project.json to specify sysroot workspace #19096

Allow rust-project.json to specify sysroot workspace #19096

Conversation

darichey commented Feb 5, 2025

darichey commented Feb 5, 2025

Veykril left a comment

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

darichey commented Feb 6, 2025 • edited Loading

Veykril left a comment

Choose a reason for hiding this comment

Veykril commented Feb 14, 2025

davidbarsky commented Feb 14, 2025

Veykril Feb 14, 2025

Choose a reason for hiding this comment

davidbarsky commented Feb 14, 2025

Veykril commented Feb 25, 2025

darichey commented Feb 26, 2025

darichey Feb 26, 2025

Choose a reason for hiding this comment

Veykril commented Feb 27, 2025

darichey commented Feb 6, 2025 •

edited

Loading