Skip to content

Commit

Permalink
Improve workspace discovery logic (#3030)
Browse files Browse the repository at this point in the history
Previously we were assuming that any crate with a path dep lives in a
workspace, and any that doesn't isn't.

This isn't correct logic.
https://doc.rust-lang.org/cargo/reference/workspaces.html describes the
specification. We do filesystem traversal to detect implicit members.
This is a little more expensive, but much more correct.

This is a precursor for supporting path dependencies - the current
incorrect logic incorrectly identifies any Cargo.toml file containing a
path dependency as expecting to live in a workspace, which breaks some
use-cases.
  • Loading branch information
illicitonion authored Dec 5, 2024
1 parent 94f3a8e commit 72556e5
Show file tree
Hide file tree
Showing 45 changed files with 806 additions and 556 deletions.
1 change: 1 addition & 0 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ use_repo(
"cui__tracing-0.1.40",
"cui__tracing-subscriber-0.3.18",
"cui__url-2.5.2",
"cui__walkdir-2.5.0",
)

crate_universe_internal_dev_deps = use_extension(
Expand Down
6 changes: 6 additions & 0 deletions crate_universe/3rdparty/crates/BUILD.bazel

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crate_universe/3rdparty/crates/BUILD.globwalk-0.8.1.bazel

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crate_universe/3rdparty/crates/BUILD.ignore-0.4.18.bazel

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions crate_universe/3rdparty/crates/defs.bzl

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions crate_universe/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 42 additions & 1 deletion crate_universe/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,46 @@
[workspace]
members = ["tools/cross_installer", "tools/urls_generator"]
exclude = ["test_data"]
exclude = [
"test_data/metadata/abspath",
"test_data/metadata/aliases",
"test_data/metadata/build_scripts",
"test_data/metadata/common",
"test_data/metadata/crate_combined_features",
"test_data/metadata/crate_optional_deps_disabled",
"test_data/metadata/crate_optional_deps_disabled_build_dep_enabled",
"test_data/metadata/crate_optional_deps_enabled",
"test_data/metadata/crate_renamed_optional_deps_disabled",
"test_data/metadata/crate_renamed_optional_deps_enabled",
"test_data/metadata/crate_types",
"test_data/metadata/example_proc_macro_dep",
"test_data/metadata/git_repos",
"test_data/metadata/has_package_metadata",
"test_data/metadata/host_specific_build_deps",
"test_data/metadata/multi_cfg_dep",
"test_data/metadata/multi_kind_proc_macro_dep",
"test_data/metadata/nested_build_dependencies",
"test_data/metadata/no_deps",
"test_data/metadata/resolver_2_deps",
"test_data/metadata/target_cfg_features",
"test_data/metadata/target_features",
"test_data/metadata/tree_data",
"test_data/metadata/workspace",
"test_data/metadata/workspace/child",
"test_data/metadata/workspace_path",
"test_data/metadata/workspace_path/child_a",
"test_data/metadata/workspace_path/child_b",
"test_data/test_data_passing_crate",
"test_data/workspace_examples/non-ws",
"test_data/workspace_examples/ws1",
"test_data/workspace_examples/ws1/ws1c1",
"test_data/workspace_examples/ws1/ws1c1/ws1c1c1",
"test_data/workspace_examples/ws1/ws1c2",
"test_data/workspace_examples/ws2",
"test_data/workspace_examples/ws2/ws2c1",
"test_data/workspace_examples/ws2/ws2excluded",
"test_data/workspace_examples/ws2/ws2excluded/ws2excluded2",
"test_data/workspace_examples/ws2/ws2excluded/ws2included",
]

[package]
name = "cargo-bazel"
Expand Down Expand Up @@ -51,6 +91,7 @@ toml = "0.8.19"
tracing = "0.1.40"
tracing-subscriber = "0.3.18"
url = "2.5.2"
walkdir = "2.5.0"

[dev-dependencies]
maplit = "1.0.2"
6 changes: 5 additions & 1 deletion crate_universe/extensions.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ def _generate_hub_and_spokes(*, module_ctx, cargo_bazel, cfg, annotations, cargo
),
)

nonhermetic_root_bazel_workspace_dir = module_ctx.path(Label("@@//:MODULE.bazel")).dirname

splicing_output_dir = tag_path.get_child("splicing-output")
splice_args = [
"splice",
Expand All @@ -121,6 +123,8 @@ def _generate_hub_and_spokes(*, module_ctx, cargo_bazel, cfg, annotations, cargo
config_file,
"--splicing-manifest",
splicing_manifest,
"--nonhermetic-root-bazel-workspace-dir",
nonhermetic_root_bazel_workspace_dir,
]
if cargo_lockfile:
splice_args.extend([
Expand Down Expand Up @@ -153,7 +157,7 @@ def _generate_hub_and_spokes(*, module_ctx, cargo_bazel, cfg, annotations, cargo
"--lockfile",
lockfile_path,
"--nonhermetic-root-bazel-workspace-dir",
module_ctx.path(Label("@@//:MODULE.bazel")).dirname,
nonhermetic_root_bazel_workspace_dir,
"--paths-to-track",
paths_to_track_file,
"--warnings-output-path",
Expand Down
2 changes: 2 additions & 0 deletions crate_universe/private/splicing_utils.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ def splice_workspace_manifest(repository_ctx, generator, cargo_lockfile, splicin
rustc,
"--cargo-lockfile",
cargo_lockfile,
"--nonhermetic-root-bazel-workspace-dir",
repository_ctx.workspace_root,
]

# Optionally set the splicing workspace directory to somewhere within the repository directory
Expand Down
1 change: 1 addition & 0 deletions crate_universe/private/srcs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ CARGO_BAZEL_SRCS = [
Label("//crate_universe:src/metadata/cargo_tree_rustc_wrapper.sh"),
Label("//crate_universe:src/metadata/dependency.rs"),
Label("//crate_universe:src/metadata/metadata_annotation.rs"),
Label("//crate_universe:src/metadata/workspace_discoverer.rs"),
Label("//crate_universe:src/rendering.rs"),
Label("//crate_universe:src/rendering/template_engine.rs"),
Label("//crate_universe:src/rendering/templates/module_bzl.j2"),
Expand Down
13 changes: 9 additions & 4 deletions crate_universe/src/cli/splice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::path::PathBuf;

use anyhow::Context;
use camino::Utf8PathBuf;
use clap::Parser;

use crate::cli::Result;
Expand Down Expand Up @@ -31,7 +32,7 @@ pub struct SpliceOptions {
/// The directory in which to build the workspace. If this argument is not
/// passed, a temporary directory will be generated.
#[clap(long)]
pub workspace_dir: Option<PathBuf>,
pub workspace_dir: Option<Utf8PathBuf>,

/// The location where the results of splicing are written.
#[clap(long)]
Expand All @@ -56,6 +57,9 @@ pub struct SpliceOptions {
/// The path to a rustc binary for use with Cargo
#[clap(long, env = "RUSTC")]
pub rustc: PathBuf,

#[clap(long)]
pub nonhermetic_root_bazel_workspace_dir: PathBuf,
}

/// Combine a set of disjoint manifests into a single workspace.
Expand All @@ -70,7 +74,8 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
Some(dir) => dir.clone(),
None => {
temp_dir = tempfile::tempdir().context("Failed to generate temporary directory")?;
temp_dir.as_ref().to_path_buf()
Utf8PathBuf::from_path_buf(temp_dir.as_ref().to_path_buf())
.unwrap_or_else(|path| panic!("Temporary directory wasn't valid UTF-8: {:?}", path))
}
};

Expand All @@ -81,7 +86,7 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {

// Splice together the manifest
let manifest_path = splicer
.splice_workspace(&cargo)
.splice_workspace(&opt.nonhermetic_root_bazel_workspace_dir)
.context("Failed to splice workspace")?;

// Generate a lockfile
Expand Down Expand Up @@ -126,7 +131,7 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
.with_context(|| {
format!(
"The path {} is expected to have a parent directory",
manifest_path.as_path_buf().display()
manifest_path.as_path_buf()
)
})?
.join("Cargo.lock");
Expand Down
8 changes: 5 additions & 3 deletions crate_universe/src/cli/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,16 +130,18 @@ pub fn vendor(opt: VendorOptions) -> Result<()> {
.resolve(&opt.workspace_dir, &output_base);

let temp_dir = tempfile::tempdir().context("Failed to create temporary directory")?;
let temp_dir_path = Utf8PathBuf::from_path_buf(temp_dir.as_ref().to_path_buf())
.unwrap_or_else(|path| panic!("Temporary directory wasn't valid UTF-8: {:?}", path));

// Generate a splicer for creating a Cargo workspace manifest
let splicer = Splicer::new(PathBuf::from(temp_dir.as_ref()), splicing_manifest)
.context("Failed to create splicer")?;
let splicer =
Splicer::new(temp_dir_path, splicing_manifest).context("Failed to create splicer")?;

let cargo = Cargo::new(opt.cargo, opt.rustc.clone());

// Splice together the manifest
let manifest_path = splicer
.splice_workspace(&cargo)
.splice_workspace(opt.nonhermetic_root_bazel_workspace_dir.as_std_path())
.context("Failed to splice workspace")?;

// Gather a cargo lockfile
Expand Down
Loading

0 comments on commit 72556e5

Please sign in to comment.