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

fix(build-std): remove hack on creating virtual std workspace #14358

Merged
merged 3 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
56 changes: 3 additions & 53 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@ use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit};
use crate::core::profiles::{Profiles, UnitFor};
use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures};
use crate::core::resolver::HasDevUnits;
use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace};
use crate::core::{PackageId, PackageSet, Resolve, Workspace};
use crate::ops::{self, Packages};
use crate::util::errors::CargoResult;
use crate::GlobalContext;
use std::collections::{HashMap, HashSet};
use std::path::PathBuf;
use std::rc::Rc;

use super::BuildConfig;

Expand Down Expand Up @@ -74,60 +73,11 @@ pub fn resolve_std<'gctx>(
}

let src_path = detect_sysroot_src_path(target_data)?;
let to_patch = [
"rustc-std-workspace-core",
"rustc-std-workspace-alloc",
"rustc-std-workspace-std",
];
let patches = to_patch
.iter()
.map(|&name| {
let source_path = SourceId::for_path(&src_path.join("library").join(name))?;
let dep = Dependency::parse(name, None, source_path)?;
Ok(dep)
})
.collect::<CargoResult<Vec<_>>>()?;
let crates_io_url = crate::sources::CRATES_IO_INDEX.parse().unwrap();
let patch = HashMap::from([(crates_io_url, patches)]);
let members = vec![
String::from("library/std"),
String::from("library/core"),
String::from("library/alloc"),
String::from("library/sysroot"),
];
let ws_config = crate::core::WorkspaceConfig::Root(crate::core::WorkspaceRootConfig::new(
&src_path,
&Some(members),
/*default_members*/ &None,
/*exclude*/ &None,
/*inheritable*/ &None,
/*custom_metadata*/ &None,
));
let virtual_manifest = crate::core::VirtualManifest::new(
Rc::default(),
Rc::new(toml_edit::ImDocument::parse("".to_owned()).expect("empty is valid TOML")),
Rc::default(),
Rc::default(),
/*replace*/ Vec::new(),
patch,
ws_config,
crate::core::Features::default(),
None,
);

let std_ws_manifest_path = src_path.join("library").join("Cargo.toml");
let gctx = ws.gctx();
// This is a delicate hack. In order for features to resolve correctly,
// the resolver needs to run a specific "current" member of the workspace.
// Thus, in order to set the features for `std`, we need to set `sysroot`
// to be the "current" member. `sysroot` is the root, and all other
// standard library crates are dependencies from there. Since none of the
// other crates need to alter their features, this should be fine, for
// now. Perhaps in the future features will be decoupled from the resolver
// and it will be easier to control feature selection.
let current_manifest = src_path.join("library/sysroot/Cargo.toml");
// TODO: Consider doing something to enforce --locked? Or to prevent the
// lock file from being written, such as setting ephemeral.
let mut std_ws = Workspace::new_virtual(src_path, current_manifest, virtual_manifest, gctx)?;
let mut std_ws = Workspace::new(&std_ws_manifest_path, gctx)?;
Copy link
Member

Choose a reason for hiding this comment

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

Can you build this workspace with --locked to avoid masking any future case where the lockfile is not up to date for whatever reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand the needs, though that requires more tweaks. I slightly lean toward get this fix first to unblock Cargo CI pipeline and then we make it --locked.

Regardless, which one is preferable?

  • Build succeeded. Lockfile got update but was written to file system.
  • Build failed because of any lockfile update.

Note that tests/build-std/main.rs should have guarded basic cases that won't update registry index, though they are not run in rust-lang/rust main CI pipeline.

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 build should fail. An outdated lockfile means that something went wrong when building the rust-src component (it should be a verbatim copy of the lockfile used while building the standard library in the rust build system, which uses --locked on CI afaik) or that the rust-src component was (accidentally) tampered with on the user's system.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking issue of it: rust-lang/wg-cargo-std-aware#38

// Don't require optional dependencies in this workspace, aka std's own
// `[dev-dependencies]`. No need for us to generate a `Resolve` which has
// those included because we'll never use them anyway.
Expand Down
19 changes: 0 additions & 19 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,25 +244,6 @@ impl<'gctx> Workspace<'gctx> {
}
}

pub fn new_virtual(
root_path: PathBuf,
current_manifest: PathBuf,
manifest: VirtualManifest,
gctx: &'gctx GlobalContext,
) -> CargoResult<Workspace<'gctx>> {
let mut ws = Workspace::new_default(current_manifest, gctx);
ws.root_manifest = Some(root_path.join("Cargo.toml"));
ws.target_dir = gctx.target_dir()?;
ws.packages
.packages
.insert(root_path, MaybePackage::Virtual(manifest));
ws.find_members()?;
ws.set_resolve_behavior()?;
// TODO: validation does not work because it walks up the directory
// tree looking for the root which is a fake file that doesn't exist.
Ok(ws)
}

/// Creates a "temporary workspace" from one package which only contains
/// that package.
///
Expand Down
27 changes: 25 additions & 2 deletions tests/build-std/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,31 @@ fn basic() {

"#]])
.run();
p.cargo("run").build_std().target_host().run();
p.cargo("test").build_std().target_host().run();
p.cargo("run")
.build_std()
.target_host()
.with_stderr_data(str![[r#"
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] `target/[HOST_TARGET]/debug/foo`

"#]])
.run();
p.cargo("test")
.build_std()
.target_host()
.with_stderr_data(str![[r#"
[COMPILING] rustc-std-workspace-std [..]
...
[COMPILING] test v0.0.0 ([..])
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `test` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] unittests src/lib.rs (target/[HOST_TARGET]/debug/deps/foo-[HASH])
[RUNNING] unittests src/main.rs (target/[HOST_TARGET]/debug/deps/foo-[HASH])
[RUNNING] tests/smoke.rs (target/[HOST_TARGET]/debug/deps/smoke-[HASH])
[DOCTEST] foo

"#]])
.run();

// Check for hack that removes dylibs.
let deps_dir = Path::new("target")
Expand Down
8 changes: 0 additions & 8 deletions tests/testsuite/mock-std/Cargo.toml

This file was deleted.

11 changes: 11 additions & 0 deletions tests/testsuite/mock-std/library/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[workspace]
resolver = "1"
members = [
"std",
"sysroot",
]

[patch.crates-io]
rustc-std-workspace-core = { path = 'rustc-std-workspace-core' }
rustc-std-workspace-alloc = { path = 'rustc-std-workspace-alloc' }
rustc-std-workspace-std = { path = 'rustc-std-workspace-std' }