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

Conversation

bakjos
Copy link

@bakjos bakjos commented Dec 6, 2024

This PR adds a new workspace toml parameter to allow picking one cargo workspace file when there are multiple in the base repo.

#3059

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Thanks for putting together - the approach generally looks reasonable, but I left a few comments. Thanks!

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?

.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?

@@ -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?

@@ -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?

@@ -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?

@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants