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

feat: Add workspace_cargo_toml attribute #3060

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions crate_universe/private/crates_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def _crates_repository_impl(repository_ctx):
repository_ctx = repository_ctx,
generator = generator,
cargo_lockfile = lockfiles.cargo,
workspace_cargo_toml = repository_ctx.path(repository_ctx.attr.workspace_cargo_toml) if repository_ctx.attr.workspace_cargo_toml else None,
splicing_manifest = splicing_manifest,
config_path = config_path,
cargo = cargo_path,
Expand Down Expand Up @@ -337,6 +338,9 @@ CARGO_BAZEL_REPIN=1 CARGO_BAZEL_REPIN_ONLY=crate_index bazel sync --only=crate_i
doc = "A set of all platform triples to consider when generating dependencies.",
default = SUPPORTED_PLATFORM_TRIPLES,
),
"workspace_cargo_toml": attr.label(
doc = "The path to the workspace `Cargo.toml` file.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document the default behaviour if this is not specified?

),
},
environ = CRATES_REPOSITORY_ENVIRON,
)
12 changes: 12 additions & 0 deletions crate_universe/private/crates_vendor.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,14 @@ def _crates_vendor_impl(ctx):
])
cargo_bazel_runfiles.extend([ctx.file.cargo_lockfile])

# Add an optional `Cargo.toml` file.
if ctx.attr.workspace_cargo_toml:
args.extend([
"--workspace-cargo-toml",
_runfiles_path(ctx.file.workspace_cargo_toml, is_windows),
])
cargo_bazel_runfiles.extend([ctx.file.workspace_cargo_toml])

# Optionally include buildifier
if ctx.attr.buildifier:
args.extend(["--buildifier", _runfiles_path(ctx.executable.buildifier, is_windows)])
Expand Down Expand Up @@ -462,6 +470,10 @@ CRATES_VENDOR_ATTRS = {
doc = "The path to a directory to write files into. Absolute paths will be treated as relative to the workspace root",
default = "crates",
),
"workspace_cargo_toml": attr.label(
doc = "The path to the workspace's `Cargo.toml` file",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document the default behaviour if this is not specified?

allow_single_file = True,
),
}

crates_vendor = rule(
Expand Down
9 changes: 8 additions & 1 deletion crate_universe/private/splicing_utils.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,14 @@ def create_splicing_manifest(repository_ctx):

return splicing_manifest

def splice_workspace_manifest(repository_ctx, generator, cargo_lockfile, splicing_manifest, config_path, cargo, rustc):
def splice_workspace_manifest(repository_ctx, generator, workspace_cargo_toml, cargo_lockfile, splicing_manifest, config_path, cargo, rustc):
"""Splice together a Cargo workspace from various other manifests and package definitions

Args:
repository_ctx (repository_ctx): The rule's context object.
generator (path): The `cargo-bazel` binary.
cargo_lockfile (path): The path to a "Cargo.lock" file.
workspace_cargo_toml (path): The path to the workspace's "Cargo.toml" file.
splicing_manifest (path): The path to a splicing manifest.
config_path: The path to the config file (containing `cargo_bazel::config::Config`.)
cargo (path): The path to a Cargo binary.
Expand Down Expand Up @@ -153,6 +154,12 @@ def splice_workspace_manifest(repository_ctx, generator, cargo_lockfile, splicin
repository_ctx.workspace_root,
]

if workspace_cargo_toml:
arguments.extend([
"--workspace-cargo-toml",
workspace_cargo_toml,
])

# Optionally set the splicing workspace directory to somewhere within the repository directory
# to improve the debugging experience.
if CARGO_BAZEL_DEBUG in repository_ctx.os.environ:
Expand Down
9 changes: 8 additions & 1 deletion crate_universe/src/cli/splice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ pub struct SpliceOptions {

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

// The path to the workspace cargo toml file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document the default behaviour if this is not specified?

#[clap(long)]
pub workspace_cargo_toml: Option<Utf8PathBuf>,
}

/// Combine a set of disjoint manifests into a single workspace.
Expand All @@ -86,7 +90,10 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {

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

// Generate a lockfile
Expand Down
9 changes: 8 additions & 1 deletion crate_universe/src/cli/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ pub struct VendorOptions {
/// You basically never want to use this value.
#[clap(long)]
pub nonhermetic_root_bazel_workspace_dir: Utf8PathBuf,

/// The path to the workspace cargo toml file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document the default behaviour if this is not specified?

#[clap(long)]
pub workspace_cargo_toml: Option<Utf8PathBuf>,
}

/// Run buildifier on a given file.
Expand Down Expand Up @@ -141,7 +145,10 @@ pub fn vendor(opt: VendorOptions) -> Result<()> {

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

// Gather a cargo lockfile
Expand Down
6 changes: 4 additions & 2 deletions crate_universe/src/metadata/workspace_discoverer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,10 @@ fn discover_workspaces_with_cache(
true
})
{
let entry =
entry.context("Failed to walk filesystem finding workspace Cargo.toml files")?;
let Ok(entry) = entry else {
// Skip errors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a surprise - what kind of errors are you running into?

continue;
};

if entry.file_name() != "Cargo.toml" {
continue;
Expand Down
48 changes: 31 additions & 17 deletions crate_universe/src/splicing/splicer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,19 @@ impl<'a> SplicerKind<'a> {
manifests: &'a BTreeMap<Utf8PathBuf, Manifest>,
splicing_manifest: &'a SplicingManifest,
nonhermetic_root_bazel_workspace_dir: &Path,
workspace_cargo_toml: Option<&Utf8Path>,
) -> Result<Self> {
if let Some((path, manifies)) = workspace_cargo_toml
.and_then(|path| manifests.get_key_value(path))
.and_then(|(path, manifest)| manifest.workspace.as_ref().map(|_| (path, manifest)))
{
return Ok(Self::Workspace {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This currently skips the "identify any member Cargo.toml files which aren't listed in the manifests" step, which is important for correctness, otherwise we may give false cache hits when those files change. Can we add in that pass (perhaps by giving discover_workspaces a "replace" arg, where we can pass in the workspaces to ignore and replace them with this one instead?

path,
manifest: manifies,
splicing_manifest,
});
}

let workspaces = discover_workspaces(
manifests.keys().cloned().collect(),
manifests,
Expand Down Expand Up @@ -503,11 +515,13 @@ impl Splicer {
pub(crate) fn splice_workspace(
&self,
nonhermetic_root_bazel_workspace_dir: &Path,
workspace_cargo_toml: Option<&Utf8Path>,
) -> Result<SplicedManifest> {
SplicerKind::new(
&self.manifests,
&self.splicing_manifest,
nonhermetic_root_bazel_workspace_dir,
workspace_cargo_toml,
)?
.splice(&self.workspace_dir)
}
Expand Down Expand Up @@ -980,7 +994,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Locate cargo
Expand Down Expand Up @@ -1023,7 +1037,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Locate cargo
Expand Down Expand Up @@ -1074,7 +1088,7 @@ mod test {
splicing_manifest,
)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"));
.splice_workspace(Path::new("/doesnotexist"), None);

assert!(workspace_manifest.is_err());

Expand Down Expand Up @@ -1104,7 +1118,7 @@ mod test {
splicing_manifest,
)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"));
.splice_workspace(Path::new("/doesnotexist"), None);

assert!(workspace_manifest.is_err());

Expand Down Expand Up @@ -1172,7 +1186,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"));
.splice_workspace(Path::new("/doesnotexist"), None);

assert!(workspace_manifest.is_err());

Expand Down Expand Up @@ -1209,7 +1223,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

let metadata = generate_metadata(workspace_manifest.as_path_buf());
Expand All @@ -1234,7 +1248,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Locate cargo
Expand Down Expand Up @@ -1271,7 +1285,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Check the default resolver version
Expand Down Expand Up @@ -1321,7 +1335,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Check the specified resolver version
Expand Down Expand Up @@ -1381,7 +1395,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Check the default resolver version
Expand Down Expand Up @@ -1423,7 +1437,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Ensure the patches match the expected value
Expand Down Expand Up @@ -1486,7 +1500,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Ensure the patches match the expected value
Expand Down Expand Up @@ -1537,7 +1551,7 @@ mod test {
let workspace_manifest =
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

// Ensure the patches match the expected value
Expand Down Expand Up @@ -1579,7 +1593,7 @@ mod test {
let workspace_root = tempfile::tempdir().unwrap();
let result = Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"));
.splice_workspace(Path::new("/doesnotexist"), None);

// Confirm conflicting patches have been detected
assert!(result.is_err());
Expand All @@ -1601,7 +1615,7 @@ mod test {
let workspace_root = tempfile::tempdir().unwrap();
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

let cargo_config = workspace_root.as_ref().join(".cargo").join("config.toml");
Expand Down Expand Up @@ -1634,7 +1648,7 @@ mod test {
let workspace_root = tempfile::tempdir().unwrap();
Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"))
.splice_workspace(Path::new("/doesnotexist"), None)
.unwrap();

let cargo_config = workspace_root.as_ref().join(".cargo").join("config.toml");
Expand All @@ -1661,7 +1675,7 @@ mod test {
let workspace_root = tempdir_utf8pathbuf(&temp_dir).join("workspace_root");
let splicing_result = Splicer::new(workspace_root.clone(), splicing_manifest)
.unwrap()
.splice_workspace(Path::new("/doesnotexist"));
.splice_workspace(Path::new("/doesnotexist"), None);

// Ensure cargo config files in parent directories lead to errors
assert!(splicing_result.is_err());
Expand Down
16 changes: 16 additions & 0 deletions crate_universe/test_data/metadata/workspace/Cargo.bazel.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[workspace]
members = [
"child",
]
resolver = "2"

[package]
name = "parent"
version = "0.1.0"
edition = "2018"

# Required to satisfy cargo but no `lib.rs` is expected to
# exist within test data.
[lib]
path = "lib.rs"

Loading
Loading