Give useful error on missing workspace manifest (#1475)
Previously if the workspace root manifest was missing, a panic would be
triggered due to unwrapping a `None` option.
Now, we suggest the missing manifest if possible, or direct you to work
it out yourself if we can't.
diff --git a/crate_universe/src/cli/splice.rs b/crate_universe/src/cli/splice.rs
index 999b7dc..f5077bf 100644
--- a/crate_universe/src/cli/splice.rs
+++ b/crate_universe/src/cli/splice.rs
@@ -70,7 +70,7 @@
let splicer = Splicer::new(splicing_dir, splicing_manifest)?;
// Splice together the manifest
- let manifest_path = splicer.splice_workspace()?;
+ let manifest_path = splicer.splice_workspace(&opt.cargo)?;
// Generate a lockfile
let cargo_lockfile = generate_lockfile(
diff --git a/crate_universe/src/cli/vendor.rs b/crate_universe/src/cli/vendor.rs
index 5a0b183..366bb68 100644
--- a/crate_universe/src/cli/vendor.rs
+++ b/crate_universe/src/cli/vendor.rs
@@ -127,7 +127,7 @@
// Splice together the manifest
let manifest_path = splicer
- .splice_workspace()
+ .splice_workspace(&opt.cargo)
.context("Failed to splice workspace")?;
// Gather a cargo lockfile
diff --git a/crate_universe/src/splicing/splicer.rs b/crate_universe/src/splicing/splicer.rs
index 1a68d32..8374df4 100644
--- a/crate_universe/src/splicing/splicer.rs
+++ b/crate_universe/src/splicing/splicer.rs
@@ -5,6 +5,7 @@
use std::path::{Path, PathBuf};
use anyhow::{bail, Context, Result};
+use cargo_metadata::MetadataCommand;
use cargo_toml::{Dependency, Manifest};
use normpath::PathExt;
@@ -44,6 +45,7 @@
pub fn new(
manifests: &'a HashMap<PathBuf, Manifest>,
splicing_manifest: &'a SplicingManifest,
+ cargo: &Path,
) -> Result<Self> {
// First check for any workspaces in the provided manifests
let workspace_owned: HashMap<&PathBuf, &Manifest> = manifests
@@ -67,7 +69,33 @@
bail!("When splicing manifests, there can only be 1 root workspace manifest");
}
+ // This is an error case - we've detected some manifests are in a workspace, but can't
+ // find it.
+ // This block is just for trying to give as useful an error message as possible in this
+ // case.
+ if workspace_roots.is_empty() {
+ let sorted_manifests: BTreeSet<_> = manifests.keys().collect();
+ for manifest_path in sorted_manifests {
+ let metadata_result = MetadataCommand::new()
+ .cargo_path(cargo)
+ .current_dir(manifest_path.parent().unwrap())
+ .manifest_path(manifest_path)
+ .no_deps()
+ .exec();
+ if let Ok(metadata) = metadata_result {
+ let label = Label::from_absolute_path(
+ metadata.workspace_root.join("Cargo.toml").as_std_path(),
+ );
+ if let Ok(label) = label {
+ bail!("Missing root workspace manifest. Please add the following label to the `manifests` key: \"{}\"", label);
+ }
+ }
+ }
+ bail!("Missing root workspace manifest. Please add the label of the workspace root to the `manifests` key");
+ }
+
// Ensure all workspace owned manifests are members of the one workspace root
+ // UNWRAP: Safe because we've checked workspace_roots isn't empty.
let (root_manifest_path, root_manifest) = workspace_roots.drain().last().unwrap();
let external_workspace_members: BTreeSet<String> = workspace_packages
.into_iter()
@@ -470,8 +498,9 @@
}
/// Build a new workspace root
- pub fn splice_workspace(&self) -> Result<SplicedManifest> {
- SplicerKind::new(&self.manifests, &self.splicing_manifest)?.splice(&self.workspace_dir)
+ pub fn splice_workspace(&self, cargo: &Path) -> Result<SplicedManifest> {
+ SplicerKind::new(&self.manifests, &self.splicing_manifest, cargo)?
+ .splice(&self.workspace_dir)
}
}
@@ -706,6 +735,10 @@
(PathBuf::from("cargo"), PathBuf::from("rustc"))
}
+ fn cargo() -> PathBuf {
+ get_cargo_and_rustc_paths().0
+ }
+
fn generate_metadata(manifest_path: &Path) -> cargo_metadata::Metadata {
let manifest_dir = manifest_path.parent().unwrap_or_else(|| {
panic!(
@@ -746,16 +779,28 @@
}
fn mock_cargo_toml(path: &Path, name: &str) -> cargo_toml::Manifest {
+ mock_cargo_toml_with_dependencies(path, name, &[])
+ }
+
+ fn mock_cargo_toml_with_dependencies(
+ path: &Path,
+ name: &str,
+ deps: &[&str],
+ ) -> cargo_toml::Manifest {
let manifest = cargo_toml::Manifest::from_str(&textwrap::dedent(&format!(
r#"
[package]
- name = "{}"
+ name = "{name}"
version = "0.0.1"
[lib]
path = "lib.rs"
+
+ [dependencies]
+ {dependencies}
"#,
- name
+ name = name,
+ dependencies = deps.join("\n")
)))
.unwrap();
@@ -809,7 +854,12 @@
.join("root_pkg")
.join(pkg)
.join("Cargo.toml");
- mock_cargo_toml(&manifest_path, pkg);
+ let deps = if pkg == &"sub_pkg_b" {
+ vec![r#"sub_pkg_a = { path = "../sub_pkg_a" }"#]
+ } else {
+ vec![]
+ };
+ mock_cargo_toml_with_dependencies(&manifest_path, pkg, &deps);
splicing_manifest.manifests.insert(
manifest_path,
@@ -842,6 +892,9 @@
let manifest_path = root_pkg.join("Cargo.toml");
fs::create_dir_all(&manifest_path.parent().unwrap()).unwrap();
fs::write(&manifest_path, toml::to_string(&manifest).unwrap()).unwrap();
+ {
+ File::create(root_pkg.join("BUILD.bazel")).unwrap();
+ }
let sub_pkg_a = root_pkg.join("sub_pkg_a");
let sub_pkg_b = root_pkg.join("sub_pkg_b");
@@ -855,7 +908,7 @@
splicing_manifest.manifests.insert(
manifest_path,
- Label::from_str("//pkg_root:Cargo.toml").unwrap(),
+ Label::from_str("//root_pkg:Cargo.toml").unwrap(),
);
(splicing_manifest, cache_dir)
@@ -977,7 +1030,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
// Ensure metadata is valid
@@ -1010,7 +1063,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
// Ensure metadata is valid
@@ -1041,7 +1094,7 @@
// Remove everything but the root manifest
splicing_manifest
.manifests
- .retain(|_, label| *label == Label::from_str("//pkg_root:Cargo.toml").unwrap());
+ .retain(|_, label| *label == Label::from_str("//root_pkg:Cargo.toml").unwrap());
assert_eq!(splicing_manifest.manifests.len(), 1);
// Splice the workspace
@@ -1049,7 +1102,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace();
+ .splice_workspace(&cargo());
assert!(workspace_manifest.is_err());
@@ -1063,6 +1116,33 @@
}
#[test]
+ fn splice_workspace_report_missing_root() {
+ let (mut splicing_manifest, _cache_dir) = mock_splicing_manifest_with_workspace();
+
+ // Remove everything but the root manifest
+ splicing_manifest
+ .manifests
+ .retain(|_, label| *label != Label::from_str("//root_pkg:Cargo.toml").unwrap());
+ assert_eq!(splicing_manifest.manifests.len(), 2);
+
+ // Splice the workspace
+ let workspace_root = tempfile::tempdir().unwrap();
+ let workspace_manifest =
+ Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
+ .unwrap()
+ .splice_workspace(&cargo());
+
+ assert!(workspace_manifest.is_err());
+
+ // Ensure both the missing manifests are mentioned in the error string
+ let err_str = format!("{:?}", &workspace_manifest);
+ assert!(
+ err_str.contains("Missing root workspace manifest")
+ && err_str.contains("//root_pkg:Cargo.toml")
+ );
+ }
+
+ #[test]
fn splice_workspace_report_external_workspace_members() {
let (mut splicing_manifest, _cache_dir) = mock_splicing_manifest_with_workspace();
@@ -1101,7 +1181,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace();
+ .splice_workspace(&cargo());
assert!(workspace_manifest.is_err());
@@ -1124,7 +1204,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
// Ensure metadata is valid
@@ -1153,7 +1233,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
// Check the default resolver version
@@ -1202,7 +1282,7 @@
let workspace_manifest =
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
// Check the specified resolver version
@@ -1253,7 +1333,7 @@
let workspace_root = tempfile::tempdir().unwrap();
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
let cargo_config = workspace_root.as_ref().join(".cargo").join("config.toml");
@@ -1286,7 +1366,7 @@
let workspace_root = tempfile::tempdir().unwrap();
Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest)
.unwrap()
- .splice_workspace()
+ .splice_workspace(&cargo())
.unwrap();
let cargo_config = workspace_root.as_ref().join(".cargo").join("config.toml");
@@ -1313,7 +1393,7 @@
let workspace_root = temp_dir.as_ref().join("workspace_root");
let splicing_result = Splicer::new(workspace_root.clone(), splicing_manifest)
.unwrap()
- .splice_workspace();
+ .splice_workspace(&cargo());
// Ensure cargo config files in parent directories lead to errors
assert!(splicing_result.is_err());
