From f12a4f78c1419fe7e1417571709cb4b04fdd0691 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 23 Aug 2020 01:43:01 +0100 Subject: [PATCH 01/18] Add additional fields to Cargo.toml [workspace] --- src/cargo/core/compiler/standard_lib.rs | 10 +- src/cargo/core/workspace.rs | 91 +++++++++++--- src/cargo/util/toml/mod.rs | 153 ++++++++++++++---------- 3 files changed, 173 insertions(+), 81 deletions(-) diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index ed3dd77336c..01519a14504 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -60,13 +60,9 @@ pub fn resolve_std<'cfg>( String::from("library/alloc"), String::from("library/test"), ]; - let ws_config = crate::core::WorkspaceConfig::Root(crate::core::WorkspaceRootConfig::new( - &src_path, - &Some(members), - /*default_members*/ &None, - /*exclude*/ &None, - /*custom_metadata*/ &None, - )); + let ws_config = crate::core::WorkspaceConfig::Root( + crate::core::WorkspaceRootConfig::from_members(&src_path, members), + ); let virtual_manifest = crate::core::VirtualManifest::new( /*replace*/ Vec::new(), patch, diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index a8f95fcddeb..607172a65a8 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -20,7 +20,10 @@ use crate::sources::PathSource; use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; use crate::util::interning::InternedString; use crate::util::paths; -use crate::util::toml::{read_manifest, TomlProfiles}; +use crate::util::toml::{ + map_deps, read_manifest, StringOrBool, TomlDependency, TomlProfiles, TomlWorkspace, + VecStringOrBool, +}; use crate::util::{Config, Filesystem}; /// The core abstraction in Cargo for working with a workspace of crates. @@ -131,6 +134,23 @@ pub struct WorkspaceRootConfig { default_members: Option>, exclude: Vec, custom_metadata: Option, + + // Properties that can be inherited by members. + dependencies: Option>, + version: Option, + authors: Option>, + description: Option, + documentation: Option, + readme: Option, + homepage: Option, + repository: Option, + license: Option, + license_file: Option, + keywords: Option>, + categories: Option>, + publish: Option, + edition: Option, + badges: Option>>, } /// An iterator over the member packages of a workspace, returned by @@ -1154,22 +1174,65 @@ impl MaybePackage { } impl WorkspaceRootConfig { - /// Creates a new Intermediate Workspace Root configuration. - pub fn new( - root_dir: &Path, - members: &Option>, - default_members: &Option>, - exclude: &Option>, - custom_metadata: &Option, - ) -> WorkspaceRootConfig { - WorkspaceRootConfig { + pub fn from_members(root_dir: &Path, members: Vec) -> WorkspaceRootConfig { + Self { root_dir: root_dir.to_path_buf(), - members: members.clone(), - default_members: default_members.clone(), - exclude: exclude.clone().unwrap_or_default(), - custom_metadata: custom_metadata.clone(), + members: Some(members), + default_members: None, + exclude: Vec::new(), + custom_metadata: None, + dependencies: None, + version: None, + authors: None, + description: None, + documentation: None, + readme: None, + homepage: None, + repository: None, + license: None, + license_file: None, + keywords: None, + categories: None, + publish: None, + edition: None, + badges: None, } } + /// Creates a new Intermediate Workspace Root configuration from a toml workspace. + pub fn from_toml_workspace( + root_dir: &Path, + config: &Config, + toml_workspace: &TomlWorkspace, + ) -> CargoResult { + let dependencies = map_deps( + config, + toml_workspace.dependencies.as_ref(), + |_d: &TomlDependency| true, + )?; + + Ok(Self { + root_dir: root_dir.to_path_buf(), + members: toml_workspace.members.clone(), + default_members: toml_workspace.default_members.clone(), + exclude: toml_workspace.exclude.clone().unwrap_or_default(), + custom_metadata: toml_workspace.metadata.clone(), + dependencies, + version: toml_workspace.version.clone(), + authors: toml_workspace.authors.clone(), + description: toml_workspace.description.clone(), + documentation: toml_workspace.documentation.clone(), + readme: toml_workspace.readme.clone(), + homepage: toml_workspace.homepage.clone(), + repository: toml_workspace.repository.clone(), + license: toml_workspace.license.clone(), + license_file: toml_workspace.license_file.clone(), + keywords: toml_workspace.keywords.clone(), + categories: toml_workspace.categories.clone(), + publish: toml_workspace.publish.clone(), + edition: toml_workspace.edition.clone(), + badges: toml_workspace.badges.clone(), + }) + } /// Checks the path against the `excluded` list. /// diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 4bb1d7f4d2b..79f1f222a67 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -76,6 +76,22 @@ fn do_read_manifest( }; let manifest = Rc::new(manifest); + + if let Some(deps) = manifest + .workspace + .as_ref() + .and_then(|ws| ws.dependencies.as_ref()) + { + for (name, dep) in deps { + if dep.is_optional() { + bail!( + "{} is optional, but workspace dependencies cannot be optional", + name + ); + } + } + } + return if manifest.project.is_some() || manifest.package.is_some() { let (mut manifest, paths) = TomlManifest::to_real_manifest(&manifest, source_id, package_root, config)?; @@ -773,6 +789,48 @@ impl<'de> de::Deserialize<'de> for VecStringOrBool { } } +pub fn map_deps( + config: &Config, + deps: Option<&BTreeMap>, + filter: impl Fn(&TomlDependency) -> bool, +) -> CargoResult>> { + let deps = match deps { + Some(deps) => deps, + None => return Ok(None), + }; + let deps = deps + .iter() + .filter(|(_k, v)| filter(v)) + .map(|(k, v)| Ok((k.clone(), map_dependency(config, v)?))) + .collect::>>()?; + Ok(Some(deps)) +} + +fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult { + match dep { + TomlDependency::Detailed(d) => { + let mut d = d.clone(); + // Path dependencies become crates.io deps. + d.path.take(); + // Same with git dependencies. + d.git.take(); + d.branch.take(); + d.tag.take(); + d.rev.take(); + // registry specifications are elaborated to the index URL + if let Some(registry) = d.registry.take() { + let src = SourceId::alt_registry(config, ®istry)?; + d.registry_index = Some(src.url().to_string()); + } + Ok(TomlDependency::Detailed(d)) + } + TomlDependency::Simple(s) => Ok(TomlDependency::Detailed(DetailedTomlDependency { + version: Some(s.clone()), + ..Default::default() + })), + } +} + /// Represents the `package`/`project` sections of a `Cargo.toml`. /// /// Note that the order of the fields matters, since this is the order they @@ -822,12 +880,30 @@ pub struct TomlProject { #[derive(Debug, Deserialize, Serialize)] pub struct TomlWorkspace { - members: Option>, + pub members: Option>, #[serde(rename = "default-members")] - default_members: Option>, - exclude: Option>, - metadata: Option, + pub default_members: Option>, + pub exclude: Option>, + pub metadata: Option, resolver: Option, + + // Properties that can be inherited by members. + pub dependencies: Option>, + pub version: Option, + pub authors: Option>, + pub description: Option, + pub documentation: Option, + pub readme: Option, + pub homepage: Option, + pub repository: Option, + pub license: Option, + #[serde(rename = "license-file")] + pub license_file: Option, + pub keywords: Option>, + pub categories: Option>, + pub publish: Option, + pub edition: Option, + pub badges: Option>>, } impl TomlProject { @@ -958,48 +1034,6 @@ impl TomlManifest { badges: self.badges.clone(), cargo_features, }); - - fn map_deps( - config: &Config, - deps: Option<&BTreeMap>, - filter: impl Fn(&TomlDependency) -> bool, - ) -> CargoResult>> { - let deps = match deps { - Some(deps) => deps, - None => return Ok(None), - }; - let deps = deps - .iter() - .filter(|(_k, v)| filter(v)) - .map(|(k, v)| Ok((k.clone(), map_dependency(config, v)?))) - .collect::>>()?; - Ok(Some(deps)) - } - - fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult { - match dep { - TomlDependency::Detailed(d) => { - let mut d = d.clone(); - // Path dependencies become crates.io deps. - d.path.take(); - // Same with git dependencies. - d.git.take(); - d.branch.take(); - d.tag.take(); - d.rev.take(); - // registry specifications are elaborated to the index URL - if let Some(registry) = d.registry.take() { - let src = SourceId::alt_registry(config, ®istry)?; - d.registry_index = Some(src.url().to_string()); - } - Ok(TomlDependency::Detailed(d)) - } - TomlDependency::Simple(s) => Ok(TomlDependency::Detailed(DetailedTomlDependency { - version: Some(s.clone()), - ..Default::default() - })), - } - } } pub fn to_real_manifest( @@ -1227,13 +1261,9 @@ impl TomlManifest { }; let workspace_config = match (me.workspace.as_ref(), project.workspace.as_ref()) { - (Some(config), None) => WorkspaceConfig::Root(WorkspaceRootConfig::new( - package_root, - &config.members, - &config.default_members, - &config.exclude, - &config.metadata, - )), + (Some(toml_workspace), None) => WorkspaceConfig::Root( + WorkspaceRootConfig::from_toml_workspace(package_root, &config, toml_workspace)?, + ), (None, root) => WorkspaceConfig::Member { root: root.cloned(), }, @@ -1412,13 +1442,9 @@ impl TomlManifest { .map(|r| ResolveBehavior::from_manifest(r)) .transpose()?; let workspace_config = match me.workspace { - Some(ref config) => WorkspaceConfig::Root(WorkspaceRootConfig::new( - root, - &config.members, - &config.default_members, - &config.exclude, - &config.metadata, - )), + Some(ref toml_workspace) => WorkspaceConfig::Root( + WorkspaceRootConfig::from_toml_workspace(root, config, toml_workspace)?, + ), None => { bail!("virtual manifests must be configured with [workspace]"); } @@ -1592,6 +1618,13 @@ impl TomlDependency { TomlDependency::Simple(..) => true, } } + + fn is_optional(&self) -> bool { + match self { + TomlDependency::Detailed(d) => d.optional.unwrap_or(false), + TomlDependency::Simple(..) => false, + } + } } impl DetailedTomlDependency { From 9967eae4ab6718ae135299f7d579886a84897aaf Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 23 Aug 2020 22:37:57 +0100 Subject: [PATCH 02/18] Add tests for additional Cargo.toml workspace-specific properties. --- tests/testsuite/deduplicate_workspace.rs | 99 ++++++++++++++++++++++++ tests/testsuite/main.rs | 1 + 2 files changed, 100 insertions(+) create mode 100644 tests/testsuite/deduplicate_workspace.rs diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs new file mode 100644 index 00000000000..10f8c0f1ce9 --- /dev/null +++ b/tests/testsuite/deduplicate_workspace.rs @@ -0,0 +1,99 @@ +//! Tests for deduplicating Cargo.toml fields with { workspace = true } +use cargo_test_support::project; + +#[cargo_test] +fn permit_additional_workspace_fields() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + version = "1.2.3" + authors = ["Rustaceans"] + description = "This is a crate" + documentation = "https://www.rust-lang.org/learn" + readme = "README.md" + homepage = "https://www.rust-lang.org" + repository = "https://github.com/example/example" + license = "MIT" + license-file = "./LICENSE" + keywords = ["cli"] + categories = ["development-tools"] + publish = false + edition = "2018" + + [workspace.badges] + gitlab = { repository = "https://gitlab.com/rust-lang/rusu", branch = "master" } + + [workspace.dependencies] + dep1 = "0.1" + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.2.0" + authors = [] + workspace = ".." + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + // Should not warn about unused fields. + .with_stderr( + "\ +[COMPILING] bar v0.2.0 ([CWD]/bar) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + + p.cargo("check").run(); + let lockfile = p.read_lockfile(); + assert!(!lockfile.contains("dep1")); +} + +#[cargo_test] +fn deny_optional_dependencies() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + dep1 = { version = "0.1", optional = true } + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.2.0" + authors = [] + workspace = ".." + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]Cargo.toml` + +Caused by: + dep1 is optional, but workspace dependencies cannot be optional +", + ) + .run(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 78379e8a01f..5cc2960bb68 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -38,6 +38,7 @@ mod cross_compile; mod cross_publish; mod custom_target; mod death; +mod deduplicate_workspace; mod dep_info; mod directory; mod doc; From 675acb60370b0aff3163067b6317a198be318e8b Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Tue, 25 Aug 2020 20:36:28 +0100 Subject: [PATCH 03/18] Implement shallow manifest cache & extract `find_workspace_root` --- src/cargo/core/compiler/standard_lib.rs | 3 +- src/cargo/core/manifest.rs | 25 ++- src/cargo/core/mod.rs | 2 +- src/cargo/core/workspace.rs | 192 +++++++++++++----------- src/cargo/util/config/mod.rs | 9 ++ src/cargo/util/toml/manifest_cache.rs | 88 +++++++++++ src/cargo/util/toml/mod.rs | 108 +++++-------- 7 files changed, 266 insertions(+), 161 deletions(-) create mode 100644 src/cargo/util/toml/manifest_cache.rs diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index 01519a14504..b4b4108c980 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -11,6 +11,7 @@ use crate::util::errors::CargoResult; use std::collections::{HashMap, HashSet}; use std::env; use std::path::PathBuf; +use std::rc::Rc; /// Parse the `-Zbuild-std` flag. pub fn parse_unstable_flag(value: Option<&str>) -> Vec { @@ -66,7 +67,7 @@ pub fn resolve_std<'cfg>( let virtual_manifest = crate::core::VirtualManifest::new( /*replace*/ Vec::new(), patch, - ws_config, + Rc::new(ws_config), /*profiles*/ None, crate::core::Features::default(), None, diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index dc69fbc08e3..3bac4043e6d 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -24,6 +24,15 @@ pub enum EitherManifest { Virtual(VirtualManifest), } +impl EitherManifest { + pub fn workspace(&self) -> Rc { + match self { + Self::Real(manifest) => Rc::clone(&manifest.workspace), + Self::Virtual(virtual_manifest) => Rc::clone(&virtual_manifest.workspace), + } + } +} + /// Contains all the information about a package, as loaded from a `Cargo.toml`. #[derive(Clone, Debug)] pub struct Manifest { @@ -40,7 +49,7 @@ pub struct Manifest { publish_lockfile: bool, replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, - workspace: WorkspaceConfig, + workspace: Rc, original: Rc, features: Features, edition: Edition, @@ -66,7 +75,7 @@ pub struct Warnings(Vec); pub struct VirtualManifest { replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, - workspace: WorkspaceConfig, + workspace: Rc, profiles: Option, warnings: Warnings, features: Features, @@ -370,7 +379,7 @@ impl Manifest { publish_lockfile: bool, replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, - workspace: WorkspaceConfig, + workspace: Rc, features: Features, edition: Edition, im_a_teapot: Option, @@ -463,8 +472,8 @@ impl Manifest { self.links.as_deref() } - pub fn workspace_config(&self) -> &WorkspaceConfig { - &self.workspace + pub fn workspace_config(&self) -> Rc { + Rc::clone(&self.workspace) } pub fn features(&self) -> &Features { @@ -538,7 +547,7 @@ impl VirtualManifest { pub fn new( replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, - workspace: WorkspaceConfig, + workspace: Rc, profiles: Option, features: Features, resolve_behavior: Option, @@ -562,8 +571,8 @@ impl VirtualManifest { &self.patch } - pub fn workspace_config(&self) -> &WorkspaceConfig { - &self.workspace + pub fn workspace_config(&self) -> Rc { + Rc::clone(&self.workspace) } pub fn profiles(&self) -> Option<&TomlProfiles> { diff --git a/src/cargo/core/mod.rs b/src/cargo/core/mod.rs index 5abab1b9cd4..3a9eeebbb1f 100644 --- a/src/cargo/core/mod.rs +++ b/src/cargo/core/mod.rs @@ -28,4 +28,4 @@ pub mod resolver; pub mod shell; pub mod source; pub mod summary; -mod workspace; +pub mod workspace; diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 607172a65a8..e8e274f4258 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -21,8 +21,8 @@ use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; use crate::util::interning::InternedString; use crate::util::paths; use crate::util::toml::{ - map_deps, read_manifest, StringOrBool, TomlDependency, TomlProfiles, TomlWorkspace, - VecStringOrBool, + map_deps, parse_manifest, read_manifest, StringOrBool, TomlDependency, TomlProfiles, + TomlWorkspace, VecStringOrBool, }; use crate::util::{Config, Filesystem}; @@ -123,6 +123,15 @@ pub enum WorkspaceConfig { Member { root: Option }, } +impl WorkspaceConfig { + pub fn is_root(&self) -> bool { + match self { + Self::Root(_) => true, + _ => false, + } + } +} + /// Intermediate configuration of a workspace root in a manifest. /// /// Knows the Workspace Root path, as well as `members` and `exclude` lists of path patterns, which @@ -177,9 +186,10 @@ impl<'cfg> Workspace<'cfg> { manifest_path ) } else { - ws.root_manifest = ws.find_root(manifest_path)?; + ws.root_manifest = find_workspace_root(manifest_path, config)?; } + ws.load_current()?; ws.custom_metadata = ws .load_workspace_config()? .and_then(|cfg| cfg.custom_metadata); @@ -267,6 +277,11 @@ impl<'cfg> Workspace<'cfg> { Ok(ws) } + fn load_current(&mut self) -> CargoResult<()> { + self.packages.load(&self.current_manifest)?; + Ok(()) + } + /// Returns the current package of this workspace. /// /// Note that this can return an error if it the current manifest is @@ -433,7 +448,7 @@ impl<'cfg> Workspace<'cfg> { // metadata. if let Some(root_path) = &self.root_manifest { let root_package = self.packages.load(root_path)?; - match root_package.workspace_config() { + match root_package.workspace_config().as_ref() { WorkspaceConfig::Root(ref root_config) => { return Ok(Some(root_config.clone())); } @@ -448,79 +463,6 @@ impl<'cfg> Workspace<'cfg> { Ok(None) } - /// Finds the root of a workspace for the crate whose manifest is located - /// at `manifest_path`. - /// - /// This will parse the `Cargo.toml` at `manifest_path` and then interpret - /// the workspace configuration, optionally walking up the filesystem - /// looking for other workspace roots. - /// - /// Returns an error if `manifest_path` isn't actually a valid manifest or - /// if some other transient error happens. - fn find_root(&mut self, manifest_path: &Path) -> CargoResult> { - fn read_root_pointer(member_manifest: &Path, root_link: &str) -> CargoResult { - let path = member_manifest - .parent() - .unwrap() - .join(root_link) - .join("Cargo.toml"); - debug!("find_root - pointer {}", path.display()); - Ok(paths::normalize_path(&path)) - }; - - { - let current = self.packages.load(manifest_path)?; - match *current.workspace_config() { - WorkspaceConfig::Root(_) => { - debug!("find_root - is root {}", manifest_path.display()); - return Ok(Some(manifest_path.to_path_buf())); - } - WorkspaceConfig::Member { - root: Some(ref path_to_root), - } => return Ok(Some(read_root_pointer(manifest_path, path_to_root)?)), - WorkspaceConfig::Member { root: None } => {} - } - } - - for path in paths::ancestors(manifest_path).skip(2) { - if path.ends_with("target/package") { - break; - } - - let ances_manifest_path = path.join("Cargo.toml"); - debug!("find_root - trying {}", ances_manifest_path.display()); - if ances_manifest_path.exists() { - match *self.packages.load(&ances_manifest_path)?.workspace_config() { - WorkspaceConfig::Root(ref ances_root_config) => { - debug!("find_root - found a root checking exclusion"); - if !ances_root_config.is_excluded(manifest_path) { - debug!("find_root - found!"); - return Ok(Some(ances_manifest_path)); - } - } - WorkspaceConfig::Member { - root: Some(ref path_to_root), - } => { - debug!("find_root - found pointer"); - return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)?)); - } - WorkspaceConfig::Member { .. } => {} - } - } - - // Don't walk across `CARGO_HOME` when we're looking for the - // workspace root. Sometimes a package will be organized with - // `CARGO_HOME` pointing inside of the workspace root or in the - // current package, but we don't want to mistakenly try to put - // crates.io crates into the workspace by accident. - if self.config.home() == path { - break; - } - } - - Ok(None) - } - /// After the root of a workspace has been located, probes for all members /// of a workspace. /// @@ -607,7 +549,7 @@ impl<'cfg> Workspace<'cfg> { } if is_path_dep && !manifest_path.parent().unwrap().starts_with(self.root()) - && self.find_root(&manifest_path)? != self.root_manifest + && find_workspace_root(&manifest_path, self.config)? != self.root_manifest { // If `manifest_path` is a path dependency outside of the workspace, // don't add it, or any of its dependencies, as a members. @@ -711,7 +653,7 @@ impl<'cfg> Workspace<'cfg> { .iter() .filter(|&member| { let config = self.packages.get(member).workspace_config(); - matches!(config, WorkspaceConfig::Root(_)) + matches!(config.as_ref(), WorkspaceConfig::Root(_)) }) .map(|member| member.parent().unwrap().to_path_buf()) .collect(); @@ -740,7 +682,7 @@ impl<'cfg> Workspace<'cfg> { fn validate_members(&mut self) -> CargoResult<()> { for member in self.members.clone() { - let root = self.find_root(&member)?; + let root = find_workspace_root(&member, self.config)?; if root == self.root_manifest { continue; } @@ -1126,11 +1068,11 @@ impl<'cfg> Packages<'cfg> { } fn load(&mut self, manifest_path: &Path) -> CargoResult<&MaybePackage> { - let key = manifest_path.parent().unwrap(); - match self.packages.entry(key.to_path_buf()) { + let key = manifest_path.parent().unwrap().to_path_buf(); + match self.packages.entry(key.clone()) { Entry::Occupied(e) => Ok(e.into_mut()), Entry::Vacant(v) => { - let source_id = SourceId::for_path(key)?; + let source_id = SourceId::for_path(&key)?; let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, self.config)?; Ok(v.insert(match manifest { @@ -1165,7 +1107,7 @@ impl<'a, 'cfg> Iterator for Members<'a, 'cfg> { } impl MaybePackage { - fn workspace_config(&self) -> &WorkspaceConfig { + fn workspace_config(&self) -> Rc { match *self { MaybePackage::Package(ref p) => p.manifest().workspace_config(), MaybePackage::Virtual(ref vm) => vm.workspace_config(), @@ -1300,3 +1242,85 @@ impl WorkspaceRootConfig { Ok(res) } } + +/// Finds the root of a workspace for the crate whose manifest is located +/// at `manifest_path`. +/// +/// This will parse the `Cargo.toml` at `manifest_path` and then interpret +/// the workspace configuration, optionally walking up the filesystem +/// looking for other workspace roots. +/// +/// Returns an error if `manifest_path` isn't actually a valid manifest or +/// if some other transient error happens. +pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult> { + fn read_root_pointer(member_manifest: &Path, root_link: &str) -> CargoResult { + let path = member_manifest + .parent() + .unwrap() + .join(root_link) + .join("Cargo.toml"); + debug!("find_workspace_root - pointer {}", path.display()); + Ok(paths::normalize_path(&path)) + }; + + fn workspace_config(manifest_path: &Path, config: &Config) -> CargoResult { + let output = parse_manifest(manifest_path, config)?; + output + .manifest + .workspace_config(manifest_path.parent().unwrap(), config) + } + + { + match workspace_config(manifest_path, config)? { + WorkspaceConfig::Root(_) => { + debug!("find_workspace_root - is root {}", manifest_path.display()); + return Ok(Some(manifest_path.to_path_buf())); + } + WorkspaceConfig::Member { + root: Some(ref path_to_root), + } => return Ok(Some(read_root_pointer(manifest_path, path_to_root)?)), + WorkspaceConfig::Member { root: None } => {} + } + } + + for path in paths::ancestors(manifest_path).skip(2) { + if path.ends_with("target/package") { + break; + } + + let ances_manifest_path = path.join("Cargo.toml"); + debug!( + "find_workspace_root - trying {}", + ances_manifest_path.display() + ); + if ances_manifest_path.exists() { + match workspace_config(&ances_manifest_path, config)? { + WorkspaceConfig::Root(ref ances_root_config) => { + debug!("find_workspace_root - found a root checking exclusion"); + if !ances_root_config.is_excluded(manifest_path) { + debug!("find_workspace_root - found!"); + return Ok(Some(ances_manifest_path)); + } + } + WorkspaceConfig::Member { + root: Some(ref path_to_root), + } => { + debug!("find_workspace_root - found pointer"); + return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)?)); + } + WorkspaceConfig::Member { .. } => {} + } + } + + // Don't walk across `CARGO_HOME` when we're looking for the + // workspace root. Sometimes a package will be organized with + // `CARGO_HOME` pointing inside of the workspace root or in the + // current package, but we don't want to mistakenly try to put + // crates.io crates into the workspace by accident. + if config.home() == path { + break; + } + } + + Ok(None) +} diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index b3d1d7b5e86..964e2a86ca1 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -176,6 +176,8 @@ pub struct Config { build_config: LazyCell, target_cfgs: LazyCell>, doc_extern_map: LazyCell, + /// Cache parsed manifests to avoid parsing twice. + manifest_cache: LazyCell>, } impl Config { @@ -247,6 +249,7 @@ impl Config { build_config: LazyCell::new(), target_cfgs: LazyCell::new(), doc_extern_map: LazyCell::new(), + manifest_cache: LazyCell::new(), } } @@ -1208,6 +1211,12 @@ impl Config { .try_borrow_with(|| self.get::("doc.extern-map")) } + pub fn manifest_cache(&self) -> RefMut<'_, cargo_toml::ManifestCache> { + self.manifest_cache + .borrow_with(|| RefCell::new(cargo_toml::ManifestCache::new())) + .borrow_mut() + } + /// Returns the `[target]` table definition for the given target triple. pub fn target_cfg_triple(&self, target: &str) -> CargoResult { target::load_target_triple(self, target) diff --git a/src/cargo/util/toml/manifest_cache.rs b/src/cargo/util/toml/manifest_cache.rs new file mode 100644 index 00000000000..f82d39066e4 --- /dev/null +++ b/src/cargo/util/toml/manifest_cache.rs @@ -0,0 +1,88 @@ +use std::collections::{ + hash_map::{Entry, HashMap}, + BTreeSet, +}; +use std::path::{Path, PathBuf}; +use std::rc::Rc; + +use super::{parse, TomlManifest}; +use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; +use crate::util::{paths, Config}; + +pub type ManifestCache = HashMap>; + +#[derive(Debug)] +pub struct ParseOutput { + pub manifest: Rc, + pub unused: BTreeSet, +} + +pub fn parse_manifest<'a>( + manifest_file: &'_ Path, + config: &'a Config, +) -> Result, ManifestError> { + let key = manifest_file.parent().unwrap().to_path_buf(); + let mut cache = config.manifest_cache(); + + match cache.entry(key.clone()) { + Entry::Occupied(e) => Ok(Rc::clone(e.get())), + Entry::Vacant(v) => { + let contents = paths::read(manifest_file) + .map_err(|err| ManifestError::new(err, manifest_file.into()))?; + + let output = deserialize(contents, manifest_file, config) + .chain_err(|| format!("failed to parse manifest at `{}`", manifest_file.display())) + .map_err(|err| ManifestError::new(err, manifest_file.into()))?; + + Ok(Rc::clone(v.insert(Rc::new(output)))) + } + } +} + +fn deserialize( + contents: String, + manifest_file: &Path, + config: &Config, +) -> CargoResult { + let pretty_filename = manifest_file + .strip_prefix(config.cwd()) + .unwrap_or(manifest_file); + + let toml = parse(&contents, pretty_filename, config)?; + let mut unused = BTreeSet::new(); + let manifest: TomlManifest = serde_ignored::deserialize(toml, |path| { + let mut key = String::new(); + stringify(&mut key, &path); + unused.insert(key); + })?; + + Ok(ParseOutput { + manifest: Rc::new(manifest), + unused, + }) +} + +fn stringify(dst: &mut String, path: &serde_ignored::Path<'_>) { + use serde_ignored::Path; + + match *path { + Path::Root => {} + Path::Seq { parent, index } => { + stringify(dst, parent); + if !dst.is_empty() { + dst.push('.'); + } + dst.push_str(&index.to_string()); + } + Path::Map { parent, ref key } => { + stringify(dst, parent); + if !dst.is_empty() { + dst.push('.'); + } + dst.push_str(key); + } + Path::Some { parent } + | Path::NewtypeVariant { parent } + | Path::NewtypeStruct { parent } => stringify(dst, parent), + } +} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 79f1f222a67..e28b2c6b60c 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt; use std::path::{Path, PathBuf}; use std::rc::Rc; @@ -27,6 +27,8 @@ use crate::util::{self, paths, validate_package_name, Config, IntoUrl}; mod targets; use self::targets::targets; +mod manifest_cache; +pub use manifest_cache::{parse_manifest, ManifestCache, ParseOutput}; pub fn read_manifest( path: &Path, @@ -38,36 +40,25 @@ pub fn read_manifest( path.display(), source_id ); - let contents = paths::read(path).map_err(|err| ManifestError::new(err, path.into()))?; - do_read_manifest(&contents, path, source_id, config) + let output = parse_manifest(path, config)?; + + do_read_manifest(&output, path, source_id, config) .chain_err(|| format!("failed to parse manifest at `{}`", path.display())) .map_err(|err| ManifestError::new(err, path.into())) } fn do_read_manifest( - contents: &str, + output: &ParseOutput, manifest_file: &Path, source_id: SourceId, config: &Config, ) -> CargoResult<(EitherManifest, Vec)> { + let manifest = &output.manifest; let package_root = manifest_file.parent().unwrap(); - let toml = { - let pretty_filename = manifest_file - .strip_prefix(config.cwd()) - .unwrap_or(manifest_file); - parse(contents, pretty_filename, config)? - }; - - let mut unused = BTreeSet::new(); - let manifest: TomlManifest = serde_ignored::deserialize(toml, |path| { - let mut key = String::new(); - stringify(&mut key, &path); - unused.insert(key); - })?; let add_unused = |warnings: &mut Warnings| { - for key in unused { + for key in &output.unused { warnings.add_warning(format!("unused manifest key: {}", key)); if key == "profiles.debug" { warnings.add_warning("use `[profile.dev]` to configure debug builds".to_string()); @@ -75,8 +66,6 @@ fn do_read_manifest( } }; - let manifest = Rc::new(manifest); - if let Some(deps) = manifest .workspace .as_ref() @@ -110,31 +99,6 @@ fn do_read_manifest( add_unused(m.warnings_mut()); Ok((EitherManifest::Virtual(m), paths)) }; - - fn stringify(dst: &mut String, path: &serde_ignored::Path<'_>) { - use serde_ignored::Path; - - match *path { - Path::Root => {} - Path::Seq { parent, index } => { - stringify(dst, parent); - if !dst.is_empty() { - dst.push('.'); - } - dst.push_str(&index.to_string()); - } - Path::Map { parent, ref key } => { - stringify(dst, parent); - if !dst.is_empty() { - dst.push('.'); - } - dst.push_str(key); - } - Path::Some { parent } - | Path::NewtypeVariant { parent } - | Path::NewtypeStruct { parent } => stringify(dst, parent), - } - } } pub fn parse(toml: &str, file: &Path, config: &Config) -> CargoResult { @@ -1260,18 +1224,8 @@ impl TomlManifest { links: project.links.clone(), }; - let workspace_config = match (me.workspace.as_ref(), project.workspace.as_ref()) { - (Some(toml_workspace), None) => WorkspaceConfig::Root( - WorkspaceRootConfig::from_toml_workspace(package_root, &config, toml_workspace)?, - ), - (None, root) => WorkspaceConfig::Member { - root: root.cloned(), - }, - (Some(..), Some(..)) => bail!( - "cannot configure both `package.workspace` and \ - `[workspace]`, only one can be specified" - ), - }; + let workspace_config = me.workspace_config(package_root, &config)?; + let profiles = me.profile.clone(); if let Some(profiles) = &profiles { profiles.validate(&features, &mut warnings)?; @@ -1329,7 +1283,7 @@ impl TomlManifest { publish_lockfile, replace, patch, - workspace_config, + Rc::new(workspace_config), features, edition, project.im_a_teapot, @@ -1441,19 +1395,17 @@ impl TomlManifest { .and_then(|ws| ws.resolver.as_deref()) .map(|r| ResolveBehavior::from_manifest(r)) .transpose()?; - let workspace_config = match me.workspace { - Some(ref toml_workspace) => WorkspaceConfig::Root( - WorkspaceRootConfig::from_toml_workspace(root, config, toml_workspace)?, - ), - None => { - bail!("virtual manifests must be configured with [workspace]"); - } - }; + + let workspace_config = me.workspace_config(root, config)?; + if !workspace_config.is_root() { + bail!("virtual manifests must be configured with [workspace]"); + } + Ok(( VirtualManifest::new( replace, patch, - workspace_config, + Rc::new(workspace_config), profiles, features, resolve_behavior, @@ -1462,6 +1414,28 @@ impl TomlManifest { )) } + pub fn workspace_config( + &self, + package_root: &Path, + config: &Config, + ) -> CargoResult { + let workspace = self.workspace.as_ref(); + let project_workspace = self.package().ok().and_then(|p| p.workspace.as_ref()); + + Ok(match (workspace, project_workspace) { + (Some(toml_workspace), None) => WorkspaceConfig::Root( + WorkspaceRootConfig::from_toml_workspace(package_root, &config, toml_workspace)?, + ), + (None, root) => WorkspaceConfig::Member { + root: root.cloned(), + }, + (Some(..), Some(..)) => bail!( + "cannot configure both `package.workspace` and \ + `[workspace]`, only one can be specified" + ), + }) + } + fn replace(&self, cx: &mut Context<'_, '_>) -> CargoResult> { if self.patch.is_some() && self.replace.is_some() { bail!("cannot specify both [replace] and [patch]"); From ecedb510ba46e3c7478805e6fcaeaeac0bcb7028 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sat, 29 Aug 2020 15:00:02 +0100 Subject: [PATCH 04/18] Add support for `MaybeWorkspace` fields. --- src/cargo/core/package.rs | 2 +- src/cargo/core/workspace.rs | 15 +- src/cargo/ops/cargo_package.rs | 6 +- src/cargo/util/toml/manifest_cache.rs | 21 +- src/cargo/util/toml/mod.rs | 332 ++++++++++++++++++++++---- tests/testsuite/build.rs | 2 +- tests/testsuite/workspaces.rs | 5 +- 7 files changed, 315 insertions(+), 68 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 1f327d75409..ed0fcbbac8e 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -240,7 +240,7 @@ impl Package { let manifest = self .manifest() .original() - .prepare_for_publish(ws, self.root())?; + .prepare_for_publish(ws, self.manifest_path())?; let toml = toml::to_string(&manifest)?; Ok(format!("{}\n{}", MANIFEST_PREAMBLE, toml)) } diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index e8e274f4258..ab539266e23 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -21,8 +21,8 @@ use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; use crate::util::interning::InternedString; use crate::util::paths; use crate::util::toml::{ - map_deps, parse_manifest, read_manifest, StringOrBool, TomlDependency, TomlProfiles, - TomlWorkspace, VecStringOrBool, + map_deps, parse_manifest, read_manifest, ParseOutput, StringOrBool, TomlDependency, + TomlProfiles, TomlWorkspace, VecStringOrBool, }; use crate::util::{Config, Filesystem}; @@ -1324,3 +1324,14 @@ pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult Ok(None) } + +pub fn find_and_parse_workspace_root( + manifest_file: &Path, + config: &Config, +) -> CargoResult> { + if let Some(root_path) = find_workspace_root(manifest_file, config)? { + Ok(Some(parse_manifest(&root_path, config)?)) + } else { + Ok(None) + } +} diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 5dc077c4b4d..d74fc606e94 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -270,16 +270,16 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult { // Convert Package -> TomlManifest -> Manifest -> Package let orig_pkg = ws.current()?; + let manifest_path = orig_pkg.manifest_path(); let toml_manifest = Rc::new( orig_pkg .manifest() .original() - .prepare_for_publish(ws, orig_pkg.root())?, + .prepare_for_publish(ws, manifest_path)?, ); - let package_root = orig_pkg.root(); let source_id = orig_pkg.package_id().source_id(); let (manifest, _nested_paths) = - TomlManifest::to_real_manifest(&toml_manifest, source_id, package_root, config)?; + TomlManifest::to_real_manifest(&toml_manifest, source_id, manifest_path, config)?; let new_pkg = Package::new(manifest, orig_pkg.manifest_path()); // Regenerate Cargo.lock using the old one as a guide. diff --git a/src/cargo/util/toml/manifest_cache.rs b/src/cargo/util/toml/manifest_cache.rs index f82d39066e4..7cda18d32f7 100644 --- a/src/cargo/util/toml/manifest_cache.rs +++ b/src/cargo/util/toml/manifest_cache.rs @@ -7,25 +7,32 @@ use std::rc::Rc; use super::{parse, TomlManifest}; use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; +use crate::util::toml::TomlWorkspace; use crate::util::{paths, Config}; -pub type ManifestCache = HashMap>; +pub type ManifestCache = HashMap; -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ParseOutput { pub manifest: Rc, - pub unused: BTreeSet, + pub unused: Rc>, +} + +impl ParseOutput { + pub fn workspace(&self) -> Option<&TomlWorkspace> { + self.manifest.workspace.as_ref() + } } pub fn parse_manifest<'a>( manifest_file: &'_ Path, config: &'a Config, -) -> Result, ManifestError> { +) -> Result { let key = manifest_file.parent().unwrap().to_path_buf(); let mut cache = config.manifest_cache(); match cache.entry(key.clone()) { - Entry::Occupied(e) => Ok(Rc::clone(e.get())), + Entry::Occupied(e) => Ok(e.get().clone()), Entry::Vacant(v) => { let contents = paths::read(manifest_file) .map_err(|err| ManifestError::new(err, manifest_file.into()))?; @@ -34,7 +41,7 @@ pub fn parse_manifest<'a>( .chain_err(|| format!("failed to parse manifest at `{}`", manifest_file.display())) .map_err(|err| ManifestError::new(err, manifest_file.into()))?; - Ok(Rc::clone(v.insert(Rc::new(output)))) + Ok(v.insert(output).clone()) } } } @@ -58,7 +65,7 @@ fn deserialize( Ok(ParseOutput { manifest: Rc::new(manifest), - unused, + unused: Rc::new(unused), }) } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index e28b2c6b60c..f147442642c 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -17,8 +17,11 @@ use crate::core::dependency::DepKind; use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings}; use crate::core::profiles::Strip; use crate::core::resolver::ResolveBehavior; +use crate::core::{ + workspace::find_and_parse_workspace_root, Edition, EitherManifest, Feature, Features, + VirtualManifest, Workspace, +}; use crate::core::{Dependency, Manifest, PackageId, Summary, Target}; -use crate::core::{Edition, EitherManifest, Feature, Features, VirtualManifest, Workspace}; use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, WorkspaceRootConfig}; use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; @@ -55,10 +58,9 @@ fn do_read_manifest( config: &Config, ) -> CargoResult<(EitherManifest, Vec)> { let manifest = &output.manifest; - let package_root = manifest_file.parent().unwrap(); let add_unused = |warnings: &mut Warnings| { - for key in &output.unused { + for key in output.unused.iter() { warnings.add_warning(format!("unused manifest key: {}", key)); if key == "profiles.debug" { warnings.add_warning("use `[profile.dev]` to configure debug builds".to_string()); @@ -83,7 +85,7 @@ fn do_read_manifest( return if manifest.project.is_some() || manifest.package.is_some() { let (mut manifest, paths) = - TomlManifest::to_real_manifest(&manifest, source_id, package_root, config)?; + TomlManifest::to_real_manifest(&manifest, source_id, manifest_file, config)?; add_unused(manifest.warnings_mut()); if manifest.targets().iter().all(|t| t.is_custom_build()) { bail!( @@ -95,7 +97,7 @@ fn do_read_manifest( Ok((EitherManifest::Real(manifest), paths)) } else { let (mut m, paths) = - TomlManifest::to_virtual_manifest(&manifest, source_id, package_root, config)?; + TomlManifest::to_virtual_manifest(&manifest, source_id, manifest_file, config)?; add_unused(m.warnings_mut()); Ok((EitherManifest::Virtual(m), paths)) }; @@ -795,6 +797,77 @@ fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult { + #[serde( + serialize_with = "serialize_workspace", + deserialize_with = "deserialize_workspace" + )] + Workspace(bool), + Defined(T), +} + +impl MaybeWorkspace { + fn from_option(value: &Option) -> Option + where + T: Clone, + { + match value { + Some(value) => Some(Self::Defined(value.clone())), + None => None, + } + } + + fn defined(&self) -> CargoResult + where + T: Clone, + { + if let Self::Defined(v) = self { + Ok(v.clone()) + } else { + bail!("defers to the workspace but could not read a value"); + } + } + + fn or(&self, workspace_value: &Option) -> CargoResult> + where + T: Clone, + { + match self { + Self::Defined(value) => Ok(Some(value.clone())), + Self::Workspace(true) => { + Ok(Some(workspace_value.clone().ok_or_else(|| { + anyhow!("defers to the workspace but could not read a value") + })?)) + } + Self::Workspace(false) => bail!("cannot specify { workspace = false } for field"), + } + } +} + +fn serialize_workspace(workspace: &bool, serializer: S) -> Result +where + S: ser::Serializer, +{ + TomlWorkspaceField { + workspace: *workspace, + } + .serialize(serializer) +} + +fn deserialize_workspace<'de, D>(deserializer: D) -> Result +where + D: de::Deserializer<'de>, +{ + Ok(TomlWorkspaceField::deserialize(deserializer)?.workspace) +} + +#[derive(Deserialize, Serialize, Debug)] +struct TomlWorkspaceField { + workspace: bool, +} + /// Represents the `package`/`project` sections of a `Cargo.toml`. /// /// Note that the order of the fields matters, since this is the order they @@ -803,19 +876,19 @@ fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult, + edition: Option>, name: InternedString, - version: semver::Version, - authors: Option>, + version: MaybeWorkspace, + authors: Option>>, build: Option, metabuild: Option, links: Option, exclude: Option>, include: Option>, - publish: Option, + publish: Option>, #[serde(rename = "publish-lockfile")] publish_lockfile: Option, - workspace: Option, + pub workspace: Option, #[serde(rename = "im-a-teapot")] im_a_teapot: Option, autobins: Option, @@ -828,18 +901,150 @@ pub struct TomlProject { default_run: Option, // Package metadata. + description: Option>, + homepage: Option>, + documentation: Option>, + readme: Option>, + keywords: Option>>, + categories: Option>>, + license: Option>, + #[serde(rename = "license-file")] + license_file: Option>, + repository: Option>, + metadata: Option, + pub resolver: Option, +} + +struct WorkspaceFields { + version: semver::Version, + authors: Option>, description: Option, - homepage: Option, documentation: Option, readme: Option, - keywords: Option>, - categories: Option>, + homepage: Option, + repository: Option, license: Option, - #[serde(rename = "license-file")] license_file: Option, - repository: Option, - metadata: Option, - resolver: Option, + keywords: Option>, + categories: Option>, + publish: Option, + edition: Option, +} + +impl TomlProject { + fn workspace_fields(&self, parse_output: Option) -> CargoResult { + if let Some(output) = parse_output { + if let Some(workspace) = output.workspace() { + let version = self + .version + .or(&workspace.version)? + .ok_or_else(|| anyhow!("no version specified"))?; + + return Ok(WorkspaceFields { + version, + + authors: if let Some(ref value) = &self.authors { + value.or(&workspace.authors)? + } else { + None + }, + + description: if let Some(ref value) = &self.description { + value.or(&workspace.description)? + } else { + None + }, + + documentation: if let Some(ref value) = &self.documentation { + value.or(&workspace.documentation)? + } else { + None + }, + + readme: if let Some(ref value) = &self.readme { + value.or(&workspace.readme)? + } else { + None + }, + + homepage: if let Some(ref value) = &self.homepage { + value.or(&workspace.homepage)? + } else { + None + }, + + repository: if let Some(ref value) = &self.repository { + value.or(&workspace.repository)? + } else { + None + }, + + license: if let Some(ref value) = &self.license { + value.or(&workspace.license)? + } else { + None + }, + + license_file: if let Some(ref value) = &self.license_file { + value.or(&workspace.license_file)? + } else { + None + }, + + keywords: if let Some(ref value) = &self.keywords { + value.or(&workspace.keywords)? + } else { + None + }, + + categories: if let Some(ref value) = &self.categories { + value.or(&workspace.categories)? + } else { + None + }, + + publish: if let Some(ref value) = &self.publish { + value.or(&workspace.publish)? + } else { + None + }, + + edition: if let Some(ref value) = &self.edition { + value.or(&workspace.edition)? + } else { + None + }, + }); + } + } + + Ok(WorkspaceFields { + version: self + .version + .defined() + .map_err(|_| anyhow!("no version specified"))?, + authors: self.authors.as_ref().map(|v| v.defined()).transpose()?, + description: self.description.as_ref().map(|v| v.defined()).transpose()?, + documentation: self + .documentation + .as_ref() + .map(|v| v.defined()) + .transpose()?, + readme: self.readme.as_ref().map(|v| v.defined()).transpose()?, + homepage: self.homepage.as_ref().map(|v| v.defined()).transpose()?, + repository: self.repository.as_ref().map(|v| v.defined()).transpose()?, + license: self.license.as_ref().map(|v| v.defined()).transpose()?, + license_file: self + .license_file + .as_ref() + .map(|v| v.defined()) + .transpose()?, + keywords: self.keywords.as_ref().map(|v| v.defined()).transpose()?, + categories: self.categories.as_ref().map(|v| v.defined()).transpose()?, + publish: self.publish.as_ref().map(|v| v.defined()).transpose()?, + edition: self.edition.as_ref().map(|v| v.defined()).transpose()?, + }) + } } #[derive(Debug, Deserialize, Serialize)] @@ -870,12 +1075,6 @@ pub struct TomlWorkspace { pub badges: Option>>, } -impl TomlProject { - pub fn to_package_id(&self, source_id: SourceId) -> CargoResult { - PackageId::new(self.name, self.version.clone(), source_id) - } -} - struct Context<'a, 'b> { pkgid: Option, deps: &'a mut Vec, @@ -892,15 +1091,28 @@ impl TomlManifest { pub fn prepare_for_publish( &self, ws: &Workspace<'_>, - package_root: &Path, + manifest_file: &Path, ) -> CargoResult { + let package_root = manifest_file.parent().unwrap(); let config = ws.config(); - let mut package = self - .package - .as_ref() - .or_else(|| self.project.as_ref()) - .unwrap() - .clone(); + let mut package = self.package().unwrap().clone(); + + let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; + let ws_fields = package.workspace_fields(parse_output)?; + package.version = MaybeWorkspace::Defined(ws_fields.version.clone()); + package.authors = MaybeWorkspace::from_option(&ws_fields.authors); + package.description = MaybeWorkspace::from_option(&ws_fields.description); + package.documentation = MaybeWorkspace::from_option(&ws_fields.documentation); + package.readme = MaybeWorkspace::from_option(&ws_fields.readme); + package.homepage = MaybeWorkspace::from_option(&ws_fields.homepage); + package.repository = MaybeWorkspace::from_option(&ws_fields.repository); + package.license = MaybeWorkspace::from_option(&ws_fields.license); + package.license_file = MaybeWorkspace::from_option(&ws_fields.license_file); + package.keywords = MaybeWorkspace::from_option(&ws_fields.keywords); + package.categories = MaybeWorkspace::from_option(&ws_fields.categories); + package.publish = MaybeWorkspace::from_option(&ws_fields.publish); + package.edition = MaybeWorkspace::from_option(&ws_fields.edition); + package.workspace = None; let mut cargo_features = self.cargo_features.clone(); package.resolver = ws.resolve_behavior().to_manifest(); @@ -915,25 +1127,26 @@ impl TomlManifest { } } } - if let Some(license_file) = &package.license_file { + + if let Some(license_file) = ws_fields.license_file { let license_path = Path::new(&license_file); let abs_license_path = paths::normalize_path(&package_root.join(license_path)); if abs_license_path.strip_prefix(package_root).is_err() { // This path points outside of the package root. `cargo package` // will copy it into the root, so adjust the path to this location. - package.license_file = Some( + package.license_file = Some(MaybeWorkspace::Defined( license_path .file_name() .unwrap() .to_str() .unwrap() .to_string(), - ); + )); } } let all = |_d: &TomlDependency| true; return Ok(TomlManifest { - package: Some(package), + package: Some(Box::new(package)), project: None, profile: self.profile.clone(), lib: self.lib.clone(), @@ -1003,9 +1216,10 @@ impl TomlManifest { pub fn to_real_manifest( me: &Rc, source_id: SourceId, - package_root: &Path, + manifest_file: &Path, config: &Config, ) -> CargoResult<(Manifest, Vec)> { + let package_root = manifest_file.parent().unwrap(); let mut nested_paths = vec![]; let mut warnings = vec![]; let mut errors = vec![]; @@ -1015,8 +1229,9 @@ impl TomlManifest { let cargo_features = me.cargo_features.as_ref().unwrap_or(&empty); let features = Features::new(cargo_features, &mut warnings)?; - let project = me.project.as_ref().or_else(|| me.package.as_ref()); - let project = project.ok_or_else(|| anyhow!("no `package` section found"))?; + let project = me.package()?; + let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; + let ws_fields = project.workspace_fields(parse_output)?; let package_name = project.name.trim(); if package_name.is_empty() { @@ -1025,9 +1240,9 @@ impl TomlManifest { validate_package_name(package_name, "package name", "")?; - let pkgid = project.to_package_id(source_id)?; + let pkgid = PackageId::new(project.name, ws_fields.version, source_id)?; - let edition = if let Some(ref edition) = project.edition { + let edition = if let Some(ref edition) = ws_fields.edition { features .require(Feature::edition()) .chain_err(|| "editions are unstable")?; @@ -1201,6 +1416,7 @@ impl TomlManifest { .collect() }) .unwrap_or_else(BTreeMap::new); + let summary = Summary::new( pkgid, deps, @@ -1210,16 +1426,16 @@ impl TomlManifest { )?; let metadata = ManifestMetadata { - description: project.description.clone(), - homepage: project.homepage.clone(), - documentation: project.documentation.clone(), - readme: readme_for_project(package_root, project), - authors: project.authors.clone().unwrap_or_default(), - license: project.license.clone(), - license_file: project.license_file.clone(), - repository: project.repository.clone(), - keywords: project.keywords.clone().unwrap_or_default(), - categories: project.categories.clone().unwrap_or_default(), + description: ws_fields.description.clone(), + homepage: ws_fields.homepage.clone(), + documentation: ws_fields.documentation.clone(), + readme: readme_for_project(package_root, ws_fields.readme), + authors: ws_fields.authors.clone().unwrap_or_default(), + license: ws_fields.license.clone(), + license_file: ws_fields.license_file.clone(), + repository: ws_fields.repository.clone(), + keywords: ws_fields.keywords.clone().unwrap_or_default(), + categories: ws_fields.categories.clone().unwrap_or_default(), badges: me.badges.clone().unwrap_or_default(), links: project.links.clone(), }; @@ -1230,7 +1446,8 @@ impl TomlManifest { if let Some(profiles) = &profiles { profiles.validate(&features, &mut warnings)?; } - let publish = match project.publish { + + let publish = match ws_fields.publish { Some(VecStringOrBool::VecString(ref vecstring)) => Some(vecstring.clone()), Some(VecStringOrBool::Bool(false)) => Some(vec![]), None | Some(VecStringOrBool::Bool(true)) => None, @@ -1314,9 +1531,10 @@ impl TomlManifest { fn to_virtual_manifest( me: &Rc, source_id: SourceId, - root: &Path, + manifest_file: &Path, config: &Config, ) -> CargoResult<(VirtualManifest, Vec)> { + let root = manifest_file.parent().unwrap(); if me.project.is_some() { bail!("this virtual manifest specifies a [project] section, which is not allowed"); } @@ -1414,6 +1632,14 @@ impl TomlManifest { )) } + pub fn package(&self) -> CargoResult<&TomlProject> { + Ok(self + .project + .as_ref() + .or_else(|| self.package.as_ref()) + .ok_or_else(|| anyhow!("no `package` section found"))?) + } + pub fn workspace_config( &self, package_root: &Path, @@ -1529,8 +1755,8 @@ impl TomlManifest { } /// Returns the name of the README file for a `TomlProject`. -fn readme_for_project(package_root: &Path, project: &TomlProject) -> Option { - match &project.readme { +fn readme_for_project(package_root: &Path, readme: Option) -> Option { + match readme { None => default_readme_from_package_root(package_root), Some(value) => match value { StringOrBool::Bool(false) => None, diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 40673506e66..12a1d45235d 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -268,7 +268,7 @@ fn cargo_compile_with_invalid_version() { [ERROR] failed to parse manifest at `[..]` Caused by: - Expected dot for key `package.version` + data did not match any variant of untagged enum MaybeWorkspace for key `package.version` ", ) .run(); diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs index e620c975d18..bae00408d46 100644 --- a/tests/testsuite/workspaces.rs +++ b/tests/testsuite/workspaces.rs @@ -377,7 +377,10 @@ fn invalid_parent_pointer() { .with_status(101) .with_stderr( "\ -error: failed to read `[..]Cargo.toml` +error: failed to parse manifest at `[..]Cargo.toml` + +Caused by: + [..] Caused by: [..] From fd5da0a62de269d771f63ca6bd6d01be883ef89d Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 30 Aug 2020 02:15:04 +0100 Subject: [PATCH 05/18] Add tests for MaybeWorkspace fields. --- crates/cargo-test-support/src/lib.rs | 13 ++ tests/testsuite/deduplicate_workspace.rs | 116 +++++++++++++---- tests/testsuite/workspaces.rs | 152 +++-------------------- 3 files changed, 124 insertions(+), 157 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 57da10dd54b..99e94a4bdce 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -1609,6 +1609,19 @@ pub fn basic_lib_manifest(name: &str) -> String { ) } +pub fn basic_workspace_manifest(name: &str, workspace: &str) -> String { + format!( + r#" + [package] + name = "{}" + version = "0.1.0" + authors = [] + workspace = "{}" + "#, + name, workspace + ) +} + pub fn path2url>(p: P) -> Url { Url::from_file_path(p).ok().unwrap() } diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs index 10f8c0f1ce9..ba26172f9e6 100644 --- a/tests/testsuite/deduplicate_workspace.rs +++ b/tests/testsuite/deduplicate_workspace.rs @@ -1,5 +1,5 @@ //! Tests for deduplicating Cargo.toml fields with { workspace = true } -use cargo_test_support::project; +use cargo_test_support::{basic_workspace_manifest, git, paths, project, publish, registry}; #[cargo_test] fn permit_additional_workspace_fields() { @@ -30,16 +30,7 @@ fn permit_additional_workspace_fields() { dep1 = "0.1" "#, ) - .file( - "bar/Cargo.toml", - r#" - [package] - name = "bar" - version = "0.2.0" - authors = [] - workspace = ".." - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "..")) .file("bar/src/main.rs", "fn main() {}") .build(); @@ -47,7 +38,7 @@ fn permit_additional_workspace_fields() { // Should not warn about unused fields. .with_stderr( "\ -[COMPILING] bar v0.2.0 ([CWD]/bar) +[COMPILING] bar v0.1.0 ([CWD]/bar) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] ", ) @@ -72,16 +63,7 @@ fn deny_optional_dependencies() { "#, ) .file("src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [package] - name = "bar" - version = "0.2.0" - authors = [] - workspace = ".." - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "..")) .file("bar/src/main.rs", "fn main() {}") .build(); @@ -97,3 +79,93 @@ Caused by: ) .run(); } + +#[cargo_test] +fn inherit_workspace_fields() { + registry::init(); + + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + version = "1.2.3" + authors = ["Rustaceans"] + description = "This is a crate" + documentation = "https://www.rust-lang.org/learn" + readme = "README.md" + homepage = "https://www.rust-lang.org" + repository = "https://github.com/example/example" + license = "MIT" + license-file = "./LICENSE" + keywords = ["cli"] + categories = ["development-tools"] + publish = true + edition = "2018" + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + workspace = ".." + version = { workspace = true } + authors = { workspace = true } + description = { workspace = true } + documentation = { workspace = true } + readme = { workspace = true } + homepage = { workspace = true } + repository = { workspace = true } + license = { workspace = true } + license-file = { workspace = true } + keywords = { workspace = true } + categories = { workspace = true } + publish = { workspace = true } + edition = { workspace = true } + "#, + ) + .file("bar/LICENSE", "license") + .file("bar/README.md", "README.md") + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("publish --token sekrit").cwd("bar").run(); + publish::validate_upload( + r#" + { + "authors": ["Rustaceans"], + "badges": {}, + "categories": ["development-tools"], + "deps": [], + "description": "This is a crate", + "documentation": "https://www.rust-lang.org/learn", + "features": {}, + "homepage": "https://www.rust-lang.org", + "keywords": ["cli"], + "license": "MIT", + "license_file": "./LICENSE", + "links": null, + "name": "bar", + "readme": "README.md", + "readme_file": "README.md", + "repository": "https://github.com/example/example", + "vers": "1.2.3" + } + "#, + "bar-1.2.3.crate", + &[ + "Cargo.lock", + "Cargo.toml", + "Cargo.toml.orig", + "src/main.rs", + "README.md", + "LICENSE", + ".cargo_vcs_info.json", + ], + ); +} diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs index bae00408d46..efe86b4973f 100644 --- a/tests/testsuite/workspaces.rs +++ b/tests/testsuite/workspaces.rs @@ -1,7 +1,9 @@ //! Tests for workspaces. use cargo_test_support::registry::Package; -use cargo_test_support::{basic_lib_manifest, basic_manifest, git, project, sleep_ms}; +use cargo_test_support::{ + basic_lib_manifest, basic_manifest, basic_workspace_manifest, git, project, sleep_ms, +}; use std::env; use std::fs; @@ -21,16 +23,7 @@ fn simple_explicit() { "#, ) .file("src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = ".." - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "..")) .file("bar/src/main.rs", "fn main() {}"); let p = p.build(); @@ -63,16 +56,7 @@ fn simple_explicit_default_members() { "#, ) .file("src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = ".." - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "..")) .file("bar/src/main.rs", "fn main() {}"); let p = p.build(); @@ -261,16 +245,7 @@ fn parent_pointer_works() { "#, ) .file("foo/src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = "../foo" - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "../foo")) .file("bar/src/main.rs", "fn main() {}") .file("bar/src/lib.rs", ""); let p = p.build(); @@ -297,16 +272,7 @@ fn same_names_in_workspace() { "#, ) .file("src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "foo" - version = "0.1.0" - authors = [] - workspace = ".." - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("foo", "..")) .file("bar/src/main.rs", "fn main() {}"); let p = p.build(); @@ -360,16 +326,7 @@ this may be fixable [..] #[cargo_test] fn invalid_parent_pointer() { let p = project() - .file( - "Cargo.toml", - r#" - [project] - name = "foo" - version = "0.1.0" - authors = [] - workspace = "foo" - "#, - ) + .file("Cargo.toml", &basic_workspace_manifest("foo", "foo")) .file("src/main.rs", "fn main() {}"); let p = p.build(); @@ -486,16 +443,7 @@ error: multiple workspace roots found in the same workspace: #[cargo_test] fn workspace_isnt_root() { let p = project() - .file( - "Cargo.toml", - r#" - [project] - name = "foo" - version = "0.1.0" - authors = [] - workspace = "bar" - "#, - ) + .file("Cargo.toml", &basic_workspace_manifest("foo", "bar")) .file("src/main.rs", "fn main() {}") .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0")) .file("bar/src/main.rs", "fn main() {}"); @@ -523,27 +471,9 @@ fn dangling_member() { "#, ) .file("src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = "../baz" - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "../baz")) .file("bar/src/main.rs", "fn main() {}") - .file( - "baz/Cargo.toml", - r#" - [project] - name = "baz" - version = "0.1.0" - authors = [] - workspace = "../baz" - "#, - ) + .file("baz/Cargo.toml", &basic_workspace_manifest("baz", "../baz")) .file("baz/src/main.rs", "fn main() {}"); let p = p.build(); @@ -562,27 +492,9 @@ actual: [..] #[cargo_test] fn cycle() { let p = project() - .file( - "Cargo.toml", - r#" - [project] - name = "foo" - version = "0.1.0" - authors = [] - workspace = "bar" - "#, - ) + .file("Cargo.toml", &basic_workspace_manifest("foo", "bar")) .file("src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = ".." - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "..")) .file("bar/src/main.rs", "fn main() {}"); let p = p.build(); @@ -1348,16 +1260,7 @@ fn relative_path_for_member_works() { "#, ) .file("foo/src/main.rs", "fn main() {}") - .file( - "bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = "../foo" - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "../foo")) .file("bar/src/main.rs", "fn main() {}"); let p = p.build(); @@ -1459,16 +1362,7 @@ fn test_in_and_out_of_workspace() { "foo/src/lib.rs", "extern crate bar; pub fn f() { bar::f() }", ) - .file( - "bar/Cargo.toml", - r#" - [project] - workspace = "../ws" - name = "bar" - version = "0.1.0" - authors = [] - "#, - ) + .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "../ws")) .file("bar/src/lib.rs", "pub fn f() { }"); let p = p.build(); @@ -1740,24 +1634,12 @@ fn glob_syntax() { .file("src/main.rs", "fn main() {}") .file( "crates/bar/Cargo.toml", - r#" - [project] - name = "bar" - version = "0.1.0" - authors = [] - workspace = "../.." - "#, + &basic_workspace_manifest("bar", "../.."), ) .file("crates/bar/src/main.rs", "fn main() {}") .file( "crates/baz/Cargo.toml", - r#" - [project] - name = "baz" - version = "0.1.0" - authors = [] - workspace = "../.." - "#, + &basic_workspace_manifest("baz", "../.."), ) .file("crates/baz/src/main.rs", "fn main() {}") .file( From 74a0d9dee6bfad90115d068dfa13dc89bbda489c Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Mon, 31 Aug 2020 16:14:03 +0100 Subject: [PATCH 06/18] Add support for `badges` field. --- src/cargo/util/toml/mod.rs | 248 ++++++++++++----------- tests/testsuite/deduplicate_workspace.rs | 11 +- 2 files changed, 137 insertions(+), 122 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index f147442642c..369669d81ce 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -249,7 +249,7 @@ pub struct TomlManifest { replace: Option>, patch: Option>>, workspace: Option, - badges: Option>>, + badges: Option>>>, } #[derive(Deserialize, Serialize, Clone, Debug, Default)] @@ -929,123 +929,10 @@ struct WorkspaceFields { categories: Option>, publish: Option, edition: Option, + badges: Option>>, } -impl TomlProject { - fn workspace_fields(&self, parse_output: Option) -> CargoResult { - if let Some(output) = parse_output { - if let Some(workspace) = output.workspace() { - let version = self - .version - .or(&workspace.version)? - .ok_or_else(|| anyhow!("no version specified"))?; - - return Ok(WorkspaceFields { - version, - - authors: if let Some(ref value) = &self.authors { - value.or(&workspace.authors)? - } else { - None - }, - - description: if let Some(ref value) = &self.description { - value.or(&workspace.description)? - } else { - None - }, - - documentation: if let Some(ref value) = &self.documentation { - value.or(&workspace.documentation)? - } else { - None - }, - - readme: if let Some(ref value) = &self.readme { - value.or(&workspace.readme)? - } else { - None - }, - - homepage: if let Some(ref value) = &self.homepage { - value.or(&workspace.homepage)? - } else { - None - }, - - repository: if let Some(ref value) = &self.repository { - value.or(&workspace.repository)? - } else { - None - }, - - license: if let Some(ref value) = &self.license { - value.or(&workspace.license)? - } else { - None - }, - - license_file: if let Some(ref value) = &self.license_file { - value.or(&workspace.license_file)? - } else { - None - }, - - keywords: if let Some(ref value) = &self.keywords { - value.or(&workspace.keywords)? - } else { - None - }, - - categories: if let Some(ref value) = &self.categories { - value.or(&workspace.categories)? - } else { - None - }, - - publish: if let Some(ref value) = &self.publish { - value.or(&workspace.publish)? - } else { - None - }, - - edition: if let Some(ref value) = &self.edition { - value.or(&workspace.edition)? - } else { - None - }, - }); - } - } - - Ok(WorkspaceFields { - version: self - .version - .defined() - .map_err(|_| anyhow!("no version specified"))?, - authors: self.authors.as_ref().map(|v| v.defined()).transpose()?, - description: self.description.as_ref().map(|v| v.defined()).transpose()?, - documentation: self - .documentation - .as_ref() - .map(|v| v.defined()) - .transpose()?, - readme: self.readme.as_ref().map(|v| v.defined()).transpose()?, - homepage: self.homepage.as_ref().map(|v| v.defined()).transpose()?, - repository: self.repository.as_ref().map(|v| v.defined()).transpose()?, - license: self.license.as_ref().map(|v| v.defined()).transpose()?, - license_file: self - .license_file - .as_ref() - .map(|v| v.defined()) - .transpose()?, - keywords: self.keywords.as_ref().map(|v| v.defined()).transpose()?, - categories: self.categories.as_ref().map(|v| v.defined()).transpose()?, - publish: self.publish.as_ref().map(|v| v.defined()).transpose()?, - edition: self.edition.as_ref().map(|v| v.defined()).transpose()?, - }) - } -} +impl TomlProject {} #[derive(Debug, Deserialize, Serialize)] pub struct TomlWorkspace { @@ -1098,7 +985,7 @@ impl TomlManifest { let mut package = self.package().unwrap().clone(); let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; - let ws_fields = package.workspace_fields(parse_output)?; + let ws_fields = self.workspace_fields(parse_output)?; package.version = MaybeWorkspace::Defined(ws_fields.version.clone()); package.authors = MaybeWorkspace::from_option(&ws_fields.authors); package.description = MaybeWorkspace::from_option(&ws_fields.description); @@ -1208,7 +1095,7 @@ impl TomlManifest { replace: None, patch: None, workspace: None, - badges: self.badges.clone(), + badges: MaybeWorkspace::define(&ws_fields.badges), cargo_features, }); } @@ -1231,7 +1118,7 @@ impl TomlManifest { let project = me.package()?; let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; - let ws_fields = project.workspace_fields(parse_output)?; + let ws_fields = me.workspace_fields(parse_output)?; let package_name = project.name.trim(); if package_name.is_empty() { @@ -1436,7 +1323,7 @@ impl TomlManifest { repository: ws_fields.repository.clone(), keywords: ws_fields.keywords.clone().unwrap_or_default(), categories: ws_fields.categories.clone().unwrap_or_default(), - badges: me.badges.clone().unwrap_or_default(), + badges: ws_fields.badges.clone().unwrap_or_default(), links: project.links.clone(), }; @@ -1632,6 +1519,127 @@ impl TomlManifest { )) } + fn workspace_fields(&self, parse_output: Option) -> CargoResult { + let package = self.package()?; + if let Some(output) = parse_output { + if let Some(workspace) = output.workspace() { + let version = package + .version + .or(&workspace.version)? + .ok_or_else(|| anyhow!("no version specified"))?; + + return Ok(WorkspaceFields { + version, + + authors: match &package.authors { + Some(value) => value.or(&workspace.authors)?, + None => None, + }, + + description: match &package.description { + Some(value) => value.or(&workspace.description)?, + None => None, + }, + + documentation: match &package.documentation { + Some(value) => value.or(&workspace.documentation)?, + None => None, + }, + + readme: match &package.readme { + Some(value) => value.or(&workspace.readme)?, + None => None, + }, + + homepage: match &package.homepage { + Some(value) => value.or(&workspace.homepage)?, + None => None, + }, + + repository: match &package.repository { + Some(value) => value.or(&workspace.repository)?, + None => None, + }, + + license: match &package.license { + Some(value) => value.or(&workspace.license)?, + None => None, + }, + + license_file: match &package.license_file { + Some(value) => value.or(&workspace.license_file)?, + None => None, + }, + + keywords: match &package.keywords { + Some(value) => value.or(&workspace.keywords)?, + None => None, + }, + + categories: match &package.categories { + Some(value) => value.or(&workspace.categories)?, + None => None, + }, + + publish: match &package.publish { + Some(value) => value.or(&workspace.publish)?, + None => None, + }, + + edition: match &package.edition { + Some(value) => value.or(&workspace.edition)?, + None => None, + }, + + badges: match &self.badges { + Some(value) => value.or(&workspace.badges)?, + None => None, + }, + }); + } + } + + Ok(WorkspaceFields { + version: package + .version + .defined() + .map_err(|_| anyhow!("no version specified"))?, + authors: package.authors.as_ref().map(|v| v.defined()).transpose()?, + description: package + .description + .as_ref() + .map(|v| v.defined()) + .transpose()?, + documentation: package + .documentation + .as_ref() + .map(|v| v.defined()) + .transpose()?, + readme: package.readme.as_ref().map(|v| v.defined()).transpose()?, + homepage: package.homepage.as_ref().map(|v| v.defined()).transpose()?, + repository: package + .repository + .as_ref() + .map(|v| v.defined()) + .transpose()?, + license: package.license.as_ref().map(|v| v.defined()).transpose()?, + license_file: package + .license_file + .as_ref() + .map(|v| v.defined()) + .transpose()?, + keywords: package.keywords.as_ref().map(|v| v.defined()).transpose()?, + categories: package + .categories + .as_ref() + .map(|v| v.defined()) + .transpose()?, + publish: package.publish.as_ref().map(|v| v.defined()).transpose()?, + edition: package.edition.as_ref().map(|v| v.defined()).transpose()?, + badges: self.badges.as_ref().map(|v| v.defined()).transpose()?, + }) + } + pub fn package(&self) -> CargoResult<&TomlProject> { Ok(self .project diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs index ba26172f9e6..e8efda4df24 100644 --- a/tests/testsuite/deduplicate_workspace.rs +++ b/tests/testsuite/deduplicate_workspace.rs @@ -24,7 +24,7 @@ fn permit_additional_workspace_fields() { edition = "2018" [workspace.badges] - gitlab = { repository = "https://gitlab.com/rust-lang/rusu", branch = "master" } + gitlab = { repository = "https://gitlab.com/rust-lang/rust", branch = "master" } [workspace.dependencies] dep1 = "0.1" @@ -105,12 +105,17 @@ fn inherit_workspace_fields() { categories = ["development-tools"] publish = true edition = "2018" + + [workspace.badges] + gitlab = { repository = "https://gitlab.com/rust-lang/rust", branch = "master" } "#, ) .file("src/main.rs", "fn main() {}") .file( "bar/Cargo.toml", r#" + badges = { workspace = true } + [package] name = "bar" workspace = ".." @@ -139,7 +144,9 @@ fn inherit_workspace_fields() { r#" { "authors": ["Rustaceans"], - "badges": {}, + "badges": { + "gitlab": { "branch": "master", "repository": "https://gitlab.com/rust-lang/rust" } + }, "categories": ["development-tools"], "deps": [], "description": "This is a crate", From baed34ce91103ab8d83d5a781627075651dbcdc1 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Mon, 31 Aug 2020 19:36:38 +0100 Subject: [PATCH 07/18] Clean up workspace field inheritance code, add more tests. --- src/cargo/util/toml/mod.rs | 268 +++++++++++------------ tests/testsuite/deduplicate_workspace.rs | 214 +++++++++++++++++- 2 files changed, 335 insertions(+), 147 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 369669d81ce..2feb67ee3d6 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -808,41 +808,50 @@ pub enum MaybeWorkspace { Defined(T), } -impl MaybeWorkspace { - fn from_option(value: &Option) -> Option - where - T: Clone, - { +impl MaybeWorkspace +where + T: std::fmt::Debug + Clone, +{ + fn from_option(value: &Option) -> Option { match value { Some(value) => Some(Self::Defined(value.clone())), None => None, } } +} - fn defined(&self) -> CargoResult - where - T: Clone, - { - if let Self::Defined(v) = self { - Ok(v.clone()) - } else { - bail!("defers to the workspace but could not read a value"); - } - } +/// Parses an optional field, defaulting to the workspace's value. +fn workspace_default<'a, T, F>( + value: Option<&'a MaybeWorkspace>, + workspace: Option<&'a TomlWorkspace>, + f: F, + label: &'_ str, +) -> CargoResult> +where + T: std::fmt::Debug + Clone, + F: FnOnce(&'a TomlWorkspace) -> &'a Option, +{ + match (value, workspace) { + (None, _) => Ok(None), + (Some(MaybeWorkspace::Defined(value)), _) => Ok(Some(value.clone())), + (Some(MaybeWorkspace::Workspace(false)), _) => Err(anyhow!( + "error reading {}: cannot specify {{ workspace = false }}", + label + )), + (Some(MaybeWorkspace::Workspace(true)), Some(ws)) => f(ws) + .clone() + .ok_or_else(|| { + anyhow!( + "error reading {0}: workspace root does not define [workspace.{0}]", + label + ) + }) + .map(|value| Some(value)), - fn or(&self, workspace_value: &Option) -> CargoResult> - where - T: Clone, - { - match self { - Self::Defined(value) => Ok(Some(value.clone())), - Self::Workspace(true) => { - Ok(Some(workspace_value.clone().ok_or_else(|| { - anyhow!("defers to the workspace but could not read a value") - })?)) - } - Self::Workspace(false) => bail!("cannot specify { workspace = false } for field"), - } + (Some(MaybeWorkspace::Workspace(true)), None) => Err(anyhow!( + "error reading {}: could not read workspace root", + label + )), } } @@ -985,7 +994,7 @@ impl TomlManifest { let mut package = self.package().unwrap().clone(); let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; - let ws_fields = self.workspace_fields(parse_output)?; + let ws_fields = self.workspace_fields(&parse_output)?; package.version = MaybeWorkspace::Defined(ws_fields.version.clone()); package.authors = MaybeWorkspace::from_option(&ws_fields.authors); package.description = MaybeWorkspace::from_option(&ws_fields.description); @@ -1095,7 +1104,7 @@ impl TomlManifest { replace: None, patch: None, workspace: None, - badges: MaybeWorkspace::define(&ws_fields.badges), + badges: MaybeWorkspace::from_option(&ws_fields.badges), cargo_features, }); } @@ -1118,7 +1127,7 @@ impl TomlManifest { let project = me.package()?; let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; - let ws_fields = me.workspace_fields(parse_output)?; + let ws_fields = me.workspace_fields(&parse_output)?; let package_name = project.name.trim(); if package_name.is_empty() { @@ -1519,124 +1528,91 @@ impl TomlManifest { )) } - fn workspace_fields(&self, parse_output: Option) -> CargoResult { + fn workspace_fields(&self, parse_output: &Option) -> CargoResult { let package = self.package()?; - if let Some(output) = parse_output { - if let Some(workspace) = output.workspace() { - let version = package - .version - .or(&workspace.version)? - .ok_or_else(|| anyhow!("no version specified"))?; - - return Ok(WorkspaceFields { - version, - - authors: match &package.authors { - Some(value) => value.or(&workspace.authors)?, - None => None, - }, - - description: match &package.description { - Some(value) => value.or(&workspace.description)?, - None => None, - }, - - documentation: match &package.documentation { - Some(value) => value.or(&workspace.documentation)?, - None => None, - }, - - readme: match &package.readme { - Some(value) => value.or(&workspace.readme)?, - None => None, - }, - - homepage: match &package.homepage { - Some(value) => value.or(&workspace.homepage)?, - None => None, - }, - - repository: match &package.repository { - Some(value) => value.or(&workspace.repository)?, - None => None, - }, - - license: match &package.license { - Some(value) => value.or(&workspace.license)?, - None => None, - }, - - license_file: match &package.license_file { - Some(value) => value.or(&workspace.license_file)?, - None => None, - }, - - keywords: match &package.keywords { - Some(value) => value.or(&workspace.keywords)?, - None => None, - }, - - categories: match &package.categories { - Some(value) => value.or(&workspace.categories)?, - None => None, - }, - - publish: match &package.publish { - Some(value) => value.or(&workspace.publish)?, - None => None, - }, - - edition: match &package.edition { - Some(value) => value.or(&workspace.edition)?, - None => None, - }, - - badges: match &self.badges { - Some(value) => value.or(&workspace.badges)?, - None => None, - }, - }); - } - } + let workspace = parse_output.as_ref().and_then(|output| output.workspace()); Ok(WorkspaceFields { - version: package - .version - .defined() - .map_err(|_| anyhow!("no version specified"))?, - authors: package.authors.as_ref().map(|v| v.defined()).transpose()?, - description: package - .description - .as_ref() - .map(|v| v.defined()) - .transpose()?, - documentation: package - .documentation - .as_ref() - .map(|v| v.defined()) - .transpose()?, - readme: package.readme.as_ref().map(|v| v.defined()).transpose()?, - homepage: package.homepage.as_ref().map(|v| v.defined()).transpose()?, - repository: package - .repository - .as_ref() - .map(|v| v.defined()) - .transpose()?, - license: package.license.as_ref().map(|v| v.defined()).transpose()?, - license_file: package - .license_file - .as_ref() - .map(|v| v.defined()) - .transpose()?, - keywords: package.keywords.as_ref().map(|v| v.defined()).transpose()?, - categories: package - .categories - .as_ref() - .map(|v| v.defined()) - .transpose()?, - publish: package.publish.as_ref().map(|v| v.defined()).transpose()?, - edition: package.edition.as_ref().map(|v| v.defined()).transpose()?, - badges: self.badges.as_ref().map(|v| v.defined()).transpose()?, + version: workspace_default( + Some(&package.version), + workspace, + |ws| &ws.version, + "version", + )? + .ok_or_else(|| anyhow!("no version specified"))?, + authors: workspace_default( + package.authors.as_ref(), + workspace, + |ws| &ws.authors, + "authors", + )?, + description: workspace_default( + package.description.as_ref(), + workspace, + |ws| &ws.description, + "description", + )?, + documentation: workspace_default( + package.documentation.as_ref(), + workspace, + |ws| &ws.documentation, + "documentation", + )?, + readme: workspace_default( + package.readme.as_ref(), + workspace, + |ws| &ws.readme, + "readme", + )?, + homepage: workspace_default( + package.homepage.as_ref(), + workspace, + |ws| &ws.homepage, + "homepage", + )?, + repository: workspace_default( + package.repository.as_ref(), + workspace, + |ws| &ws.repository, + "repository", + )?, + license: workspace_default( + package.license.as_ref(), + workspace, + |ws| &ws.license, + "license", + )?, + license_file: workspace_default( + package.license_file.as_ref(), + workspace, + |ws| &ws.license_file, + "license_file", + )?, + keywords: workspace_default( + package.keywords.as_ref(), + workspace, + |ws| &ws.keywords, + "keywords", + )?, + categories: workspace_default( + package.categories.as_ref(), + workspace, + |ws| &ws.categories, + "categories", + )?, + publish: workspace_default( + package.publish.as_ref(), + workspace, + |ws| &ws.publish, + "publish", + )?, + edition: workspace_default( + package.edition.as_ref(), + workspace, + |ws| &ws.edition, + "edition", + )?, + badges: workspace_default(self.badges.as_ref(), workspace, |ws| &ws.badges, "badges")?, }) } diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs index e8efda4df24..b9f500f8539 100644 --- a/tests/testsuite/deduplicate_workspace.rs +++ b/tests/testsuite/deduplicate_workspace.rs @@ -71,7 +71,7 @@ fn deny_optional_dependencies() { .with_status(101) .with_stderr( "\ -[ERROR] failed to parse manifest at `[..]Cargo.toml` +[ERROR] failed to parse manifest at `[..]foo/Cargo.toml` Caused by: dep1 is optional, but workspace dependencies cannot be optional @@ -176,3 +176,215 @@ fn inherit_workspace_fields() { ], ); } + +#[cargo_test] +fn inherit_own_workspace_fields() { + registry::init(); + + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "Cargo.toml", + r#" + badges = { workspace = true } + + [package] + name = "foo" + version = { workspace = true } + authors = { workspace = true } + description = { workspace = true } + documentation = { workspace = true } + readme = { workspace = true } + homepage = { workspace = true } + repository = { workspace = true } + license = { workspace = true } + license-file = { workspace = true } + keywords = { workspace = true } + categories = { workspace = true } + publish = { workspace = true } + edition = { workspace = true } + + [workspace] + members = [] + version = "1.2.3" + authors = ["Rustaceans"] + description = "This is a crate" + documentation = "https://www.rust-lang.org/learn" + readme = "README.md" + homepage = "https://www.rust-lang.org" + repository = "https://github.com/example/example" + license = "MIT" + license-file = "./LICENSE" + keywords = ["cli"] + categories = ["development-tools"] + publish = true + edition = "2018" + + [workspace.badges] + gitlab = { repository = "https://gitlab.com/rust-lang/rust", branch = "master" } + "#, + ) + .file("src/main.rs", "fn main() {}") + .file("LICENSE", "license") + .file("README.md", "README.md") + .build(); + + p.cargo("publish --token sekrit").run(); + publish::validate_upload( + r#" + { + "authors": ["Rustaceans"], + "badges": { + "gitlab": { "branch": "master", "repository": "https://gitlab.com/rust-lang/rust" } + }, + "categories": ["development-tools"], + "deps": [], + "description": "This is a crate", + "documentation": "https://www.rust-lang.org/learn", + "features": {}, + "homepage": "https://www.rust-lang.org", + "keywords": ["cli"], + "license": "MIT", + "license_file": "./LICENSE", + "links": null, + "name": "foo", + "readme": "README.md", + "readme_file": "README.md", + "repository": "https://github.com/example/example", + "vers": "1.2.3" + } + "#, + "foo-1.2.3.crate", + &[ + "Cargo.lock", + "Cargo.toml", + "Cargo.toml.orig", + "src/main.rs", + "README.md", + "LICENSE", + ".cargo_vcs_info.json", + ], + ); +} + +#[cargo_test] +fn error_inherit_from_undefined_field() { + registry::init(); + + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + workspace = ".." + version = "1.2.3" + authors = ["rustaceans"] + description = { workspace = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .cwd("bar") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + error reading description: workspace root does not define [workspace.description] +", + ) + .run(); +} + +#[cargo_test] +fn error_workspace_false() { + registry::init(); + + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + workspace = ".." + version = "1.2.3" + authors = ["rustaceans"] + description = { workspace = false } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .cwd("bar") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + error reading description: cannot specify { workspace = false } +", + ) + .run(); +} + +#[cargo_test] +fn error_no_root_workspace() { + registry::init(); + + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + workspace = ".." + version = "1.2.3" + authors = ["rustaceans"] + description = { workspace = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .cwd("bar") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + error reading description: could not read workspace root +", + ) + .run(); +} From 38f7484d7d8871f2b8554110d62bc922e9a429c9 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Tue, 1 Sep 2020 01:05:43 +0100 Subject: [PATCH 08/18] Minor type fixes. --- src/cargo/util/toml/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 2feb67ee3d6..fd6f89fd967 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -921,7 +921,7 @@ pub struct TomlProject { license_file: Option>, repository: Option>, metadata: Option, - pub resolver: Option, + resolver: Option, } struct WorkspaceFields { @@ -1042,7 +1042,7 @@ impl TomlManifest { } let all = |_d: &TomlDependency| true; return Ok(TomlManifest { - package: Some(Box::new(package)), + package: Some(package), project: None, profile: self.profile.clone(), lib: self.lib.clone(), @@ -1616,7 +1616,7 @@ impl TomlManifest { }) } - pub fn package(&self) -> CargoResult<&TomlProject> { + pub fn package(&self) -> CargoResult<&Box> { Ok(self .project .as_ref() From 22ebec2f59c4b29ead75864c1f9f07e119690add Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Wed, 2 Sep 2020 19:04:18 +0100 Subject: [PATCH 09/18] Better errors when parsing MaybeWorkspace-wrapped fields. --- src/cargo/util/toml/mod.rs | 153 ++++++++++++++++++----- tests/testsuite/build.rs | 2 +- tests/testsuite/deduplicate_workspace.rs | 37 +++++- 3 files changed, 162 insertions(+), 30 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index fd6f89fd967..40ad252d653 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1,5 +1,6 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt; +use std::marker::PhantomData; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::str; @@ -8,7 +9,7 @@ use anyhow::{anyhow, bail}; use cargo_platform::Platform; use log::{debug, trace}; use semver::{self, VersionReq}; -use serde::de; +use serde::de::{self, IntoDeserializer}; use serde::ser; use serde::{Deserialize, Serialize}; use url::Url; @@ -249,6 +250,7 @@ pub struct TomlManifest { replace: Option>, patch: Option>>, workspace: Option, + #[serde(deserialize_with = "deserialize_workspace_badges", default)] badges: Option>>>, } @@ -797,17 +799,134 @@ fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult { - #[serde( - serialize_with = "serialize_workspace", - deserialize_with = "deserialize_workspace" - )] Workspace(bool), Defined(T), } +impl<'de, T> de::Deserialize<'de> for MaybeWorkspace +where + T: Deserialize<'de>, +{ + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + struct MaybeWorkspaceVisitor { + marker: PhantomData MaybeWorkspace>, + } + + impl<'de, T> de::Visitor<'de> for MaybeWorkspaceVisitor + where + T: de::Deserialize<'de>, + { + type Value = MaybeWorkspace; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("{ workspace: true } or a valid value") + } + + fn visit_bool(self, v: bool) -> Result + where + E: de::Error, + { + T::deserialize(v.into_deserializer()).map(MaybeWorkspace::Defined) + } + + fn visit_str(self, s: &str) -> Result + where + E: de::Error, + { + T::deserialize(s.into_deserializer()).map(MaybeWorkspace::Defined) + } + + fn visit_i64(self, numeric: i64) -> Result + where + E: de::Error, + { + T::deserialize(numeric.into_deserializer()).map(MaybeWorkspace::Defined) + } + + fn visit_f64(self, numeric: f64) -> Result + where + E: de::Error, + { + T::deserialize(numeric.into_deserializer()).map(MaybeWorkspace::Defined) + } + + fn visit_u64(self, numeric: u64) -> Result + where + E: de::Error, + { + T::deserialize(numeric.into_deserializer()).map(MaybeWorkspace::Defined) + } + + fn visit_seq(self, seq: V) -> Result + where + V: de::SeqAccess<'de>, + { + let svd = de::value::SeqAccessDeserializer::new(seq); + T::deserialize(svd).map(MaybeWorkspace::Defined) + } + + fn visit_map(self, map: V) -> Result + where + V: de::MapAccess<'de>, + { + let mvd = de::value::MapAccessDeserializer::new(map); + TomlWorkspaceField::deserialize(mvd).and_then(|t| { + if t.workspace { + Ok(MaybeWorkspace::Workspace(true)) + } else { + Err(de::Error::custom("workspace cannot be false")) + } + }) + } + } + + deserializer.deserialize_any(MaybeWorkspaceVisitor { + marker: PhantomData, + }) + } +} + +#[derive(Debug, Deserialize)] +#[serde(untagged)] +enum MaybeWorkspaceBadge { + Workspace(TomlWorkspaceField), + Defined(BTreeMap>), +} + +/// This exists only to provide a nicer error message. +fn deserialize_workspace_badges<'de, D>( + deserializer: D, +) -> Result>>>, D::Error> +where + D: de::Deserializer<'de>, +{ + match Option::deserialize(deserializer) { + Ok(None) => Ok(None), + Ok(Some(MaybeWorkspaceBadge::Defined(badges))) => Ok(Some(MaybeWorkspace::Defined(badges))), + Ok(Some(MaybeWorkspaceBadge::Workspace(ws))) if ws.workspace => { + Ok(Some(MaybeWorkspace::Workspace(true))) + } + Ok(Some(MaybeWorkspaceBadge::Workspace(_))) => { + Err(de::Error::custom("workspace cannot be false")) + } + + Err(_) => Err(de::Error::custom( + "expected a table of badges or { workspace = true }", + )), + } +} + +#[derive(Deserialize, Serialize, Debug)] +struct TomlWorkspaceField { + workspace: bool, +} + impl MaybeWorkspace where T: std::fmt::Debug + Clone, @@ -855,28 +974,6 @@ where } } -fn serialize_workspace(workspace: &bool, serializer: S) -> Result -where - S: ser::Serializer, -{ - TomlWorkspaceField { - workspace: *workspace, - } - .serialize(serializer) -} - -fn deserialize_workspace<'de, D>(deserializer: D) -> Result -where - D: de::Deserializer<'de>, -{ - Ok(TomlWorkspaceField::deserialize(deserializer)?.workspace) -} - -#[derive(Deserialize, Serialize, Debug)] -struct TomlWorkspaceField { - workspace: bool, -} - /// Represents the `package`/`project` sections of a `Cargo.toml`. /// /// Note that the order of the fields matters, since this is the order they diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 12a1d45235d..40673506e66 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -268,7 +268,7 @@ fn cargo_compile_with_invalid_version() { [ERROR] failed to parse manifest at `[..]` Caused by: - data did not match any variant of untagged enum MaybeWorkspace for key `package.version` + Expected dot for key `package.version` ", ) .run(); diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs index b9f500f8539..2aa0e969c00 100644 --- a/tests/testsuite/deduplicate_workspace.rs +++ b/tests/testsuite/deduplicate_workspace.rs @@ -348,7 +348,7 @@ fn error_workspace_false() { [ERROR] failed to parse manifest at `[CWD]/Cargo.toml` Caused by: - error reading description: cannot specify { workspace = false } + workspace cannot be false for key `package.description` ", ) .run(); @@ -388,3 +388,38 @@ Caused by: ) .run(); } + +#[cargo_test] +fn error_badges_wrapping() { + registry::init(); + + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "Cargo.toml", + r#" + [package] + name = "bar" + version = "1.2.3" + authors = ["rustaceans"] + + [badges] + gitlab = "1.2.3" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + expected a table of badges or { workspace = true } for key `badges` +", + ) + .run(); +} From 8b995c3c3be4ebadc65e98a40c3b757ae5fa0b3f Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 6 Sep 2020 09:59:41 +0100 Subject: [PATCH 10/18] Refactor into DefinedTomlManifest and DefinedTomlProject structs. --- src/cargo/core/manifest.rs | 8 +- src/cargo/core/package.rs | 4 +- src/cargo/ops/cargo_package.rs | 14 +- src/cargo/util/toml/mod.rs | 535 ++++++++++++++++++++------------- src/cargo/util/toml/targets.rs | 7 +- 5 files changed, 334 insertions(+), 234 deletions(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 3bac4043e6d..d1bc9cf24b5 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -16,7 +16,7 @@ use crate::core::{Dependency, PackageId, PackageIdSpec, SourceId, Summary}; use crate::core::{Edition, Feature, Features, WorkspaceConfig}; use crate::util::errors::*; use crate::util::interning::InternedString; -use crate::util::toml::{TomlManifest, TomlProfiles}; +use crate::util::toml::{DefinedTomlManifest, TomlProfiles}; use crate::util::{short_hash, Config, Filesystem}; pub enum EitherManifest { @@ -50,7 +50,7 @@ pub struct Manifest { replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, workspace: Rc, - original: Rc, + original: Rc, features: Features, edition: Edition, im_a_teapot: Option, @@ -384,7 +384,7 @@ impl Manifest { edition: Edition, im_a_teapot: Option, default_run: Option, - original: Rc, + original: Rc, metabuild: Option>, resolve_behavior: Option, ) -> Manifest { @@ -462,7 +462,7 @@ impl Manifest { pub fn replace(&self) -> &[(PackageIdSpec, Dependency)] { &self.replace } - pub fn original(&self) -> &TomlManifest { + pub fn original(&self) -> &DefinedTomlManifest { &self.original } pub fn patch(&self) -> &HashMap> { diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index ed0fcbbac8e..43f1d3f38b8 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -240,7 +240,9 @@ impl Package { let manifest = self .manifest() .original() - .prepare_for_publish(ws, self.manifest_path())?; + .prepare_for_publish(ws, self.manifest_path())? + .into_toml_manifest(); + let toml = toml::to_string(&manifest)?; Ok(format!("{}\n{}", MANIFEST_PREAMBLE, toml)) } diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index d74fc606e94..3a2179e3783 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -3,7 +3,6 @@ use std::fs::{self, File}; use std::io::prelude::*; use std::io::SeekFrom; use std::path::{Path, PathBuf}; -use std::rc::Rc; use std::sync::Arc; use std::time::SystemTime; @@ -18,7 +17,6 @@ use crate::core::{Package, PackageId, PackageSet, Resolve, Source, SourceId}; use crate::sources::PathSource; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::paths; -use crate::util::toml::TomlManifest; use crate::util::{self, restricted_names, Config, FileLock}; use crate::{drop_println, ops}; @@ -271,15 +269,13 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult { // Convert Package -> TomlManifest -> Manifest -> Package let orig_pkg = ws.current()?; let manifest_path = orig_pkg.manifest_path(); - let toml_manifest = Rc::new( - orig_pkg - .manifest() - .original() - .prepare_for_publish(ws, manifest_path)?, - ); + let toml_manifest = orig_pkg + .manifest() + .original() + .prepare_for_publish(ws, manifest_path)?; let source_id = orig_pkg.package_id().source_id(); let (manifest, _nested_paths) = - TomlManifest::to_real_manifest(&toml_manifest, source_id, manifest_path, config)?; + toml_manifest.into_real_manifest(source_id, manifest_path, config)?; let new_pkg = Package::new(manifest, orig_pkg.manifest_path()); // Regenerate Cargo.lock using the old one as a guide. diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 40ad252d653..e05b35ba21a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -18,11 +18,9 @@ use crate::core::dependency::DepKind; use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings}; use crate::core::profiles::Strip; use crate::core::resolver::ResolveBehavior; -use crate::core::{ - workspace::find_and_parse_workspace_root, Edition, EitherManifest, Feature, Features, - VirtualManifest, Workspace, -}; +use crate::core::workspace::find_workspace_root; use crate::core::{Dependency, Manifest, PackageId, Summary, Target}; +use crate::core::{Edition, EitherManifest, Feature, Features, VirtualManifest, Workspace}; use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, WorkspaceRootConfig}; use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; @@ -58,7 +56,11 @@ fn do_read_manifest( source_id: SourceId, config: &Config, ) -> CargoResult<(EitherManifest, Vec)> { - let manifest = &output.manifest; + let manifest = DefinedTomlManifest::from_toml_manifest( + TomlManifest::clone(&output.manifest), + manifest_file, + config, + )?; let add_unused = |warnings: &mut Warnings| { for key in output.unused.iter() { @@ -84,9 +86,9 @@ fn do_read_manifest( } } - return if manifest.project.is_some() || manifest.package.is_some() { + return if manifest.package.is_some() { let (mut manifest, paths) = - TomlManifest::to_real_manifest(&manifest, source_id, manifest_file, config)?; + manifest.into_real_manifest(source_id, manifest_file, config)?; add_unused(manifest.warnings_mut()); if manifest.targets().iter().all(|t| t.is_custom_build()) { bail!( @@ -97,8 +99,7 @@ fn do_read_manifest( } Ok((EitherManifest::Real(manifest), paths)) } else { - let (mut m, paths) = - TomlManifest::to_virtual_manifest(&manifest, source_id, manifest_file, config)?; + let (mut m, paths) = manifest.into_virtual_manifest(source_id, manifest_file, config)?; add_unused(m.warnings_mut()); Ok((EitherManifest::Virtual(m), paths)) }; @@ -226,7 +227,7 @@ pub struct DetailedTomlDependency { public: Option, } -#[derive(Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[serde(rename_all = "kebab-case")] pub struct TomlManifest { cargo_features: Option>, @@ -254,6 +255,98 @@ pub struct TomlManifest { badges: Option>>>, } +impl TomlManifest { + pub fn workspace_config( + &self, + package_root: &Path, + config: &Config, + ) -> CargoResult { + let workspace = self.workspace.as_ref(); + let project_workspace = self + .project + .as_ref() + .or_else(|| self.package.as_ref()) + .and_then(|p| p.workspace.as_ref()); + + Ok(match (workspace, project_workspace) { + (Some(toml_workspace), None) => WorkspaceConfig::Root( + WorkspaceRootConfig::from_toml_workspace(package_root, &config, toml_workspace)?, + ), + (None, root) => WorkspaceConfig::Member { + root: root.cloned(), + }, + (Some(..), Some(..)) => bail!( + "cannot configure both `package.workspace` and \ + `[workspace]`, only one can be specified" + ), + }) + } +} + +#[derive(Debug, Clone)] +pub struct DefinedTomlManifest { + cargo_features: Option>, + package: Option, + profile: Option, + lib: Option, + bin: Option>, + example: Option>, + test: Option>, + bench: Option>, + dependencies: Option>, + dev_dependencies: Option>, + build_dependencies: Option>, + features: Option>>, + target: Option>, + replace: Option>, + patch: Option>>, + workspace: Option, + badges: Option>>, +} + +impl DefinedTomlManifest { + fn from_toml_manifest( + manifest: TomlManifest, + manifest_file: &Path, + config: &Config, + ) -> CargoResult { + let output = find_workspace_root(manifest_file, config)? + .map(|root_path| parse_manifest(&root_path, config)) + .transpose()?; + + let workspace = output.as_ref().and_then(|output| output.workspace()); + + let package = manifest + .package + .or(manifest.project) + .map(|p| *p) + .map(|p| DefinedTomlPackage::from_toml_project(p, workspace)) + .transpose()?; + + let badges = ws_default(manifest.badges, workspace, |ws| &ws.badges, "badges")?; + + Ok(Self { + cargo_features: manifest.cargo_features, + package, + profile: manifest.profile, + lib: manifest.lib, + bin: manifest.bin, + example: manifest.example, + test: manifest.test, + bench: manifest.bench, + dependencies: manifest.dependencies, + dev_dependencies: manifest.dev_dependencies, + build_dependencies: manifest.build_dependencies, + features: manifest.features, + target: manifest.target, + replace: manifest.replace, + patch: manifest.patch, + workspace: manifest.workspace, + badges, + }) + } +} + #[derive(Deserialize, Serialize, Clone, Debug, Default)] pub struct TomlProfiles(BTreeMap); @@ -824,6 +917,8 @@ where { type Value = MaybeWorkspace; + /// The `visit_foo` methods should cover all the possibilities, so we should in theory + /// never fallback to this error message. fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { formatter.write_str("{ workspace: true } or a valid value") } @@ -940,19 +1035,19 @@ where } /// Parses an optional field, defaulting to the workspace's value. -fn workspace_default<'a, T, F>( - value: Option<&'a MaybeWorkspace>, - workspace: Option<&'a TomlWorkspace>, +fn ws_default( + value: Option>, + workspace: Option<&TomlWorkspace>, f: F, - label: &'_ str, + label: &str, ) -> CargoResult> where T: std::fmt::Debug + Clone, - F: FnOnce(&'a TomlWorkspace) -> &'a Option, + F: FnOnce(&TomlWorkspace) -> &Option, { match (value, workspace) { (None, _) => Ok(None), - (Some(MaybeWorkspace::Defined(value)), _) => Ok(Some(value.clone())), + (Some(MaybeWorkspace::Defined(value)), _) => Ok(Some(value)), (Some(MaybeWorkspace::Workspace(false)), _) => Err(anyhow!( "error reading {}: cannot specify {{ workspace = false }}", label @@ -994,7 +1089,7 @@ pub struct TomlProject { publish: Option>, #[serde(rename = "publish-lockfile")] publish_lockfile: Option, - pub workspace: Option, + workspace: Option, #[serde(rename = "im-a-teapot")] im_a_teapot: Option, autobins: Option, @@ -1021,26 +1116,107 @@ pub struct TomlProject { resolver: Option, } -struct WorkspaceFields { +#[derive(Clone, Debug)] +struct DefinedTomlPackage { + edition: Option, + name: InternedString, version: semver::Version, authors: Option>, + build: Option, + metabuild: Option, + links: Option, + exclude: Option>, + include: Option>, + publish: Option, + publish_lockfile: Option, + pub workspace: Option, + im_a_teapot: Option, + autobins: Option, + autoexamples: Option, + autotests: Option, + autobenches: Option, + namespaced_features: Option, + default_run: Option, + + // Package metadata. description: Option, + homepage: Option, documentation: Option, readme: Option, - homepage: Option, - repository: Option, - license: Option, - license_file: Option, keywords: Option>, categories: Option>, - publish: Option, - edition: Option, - badges: Option>>, + license: Option, + license_file: Option, + repository: Option, + metadata: Option, + resolver: Option, } -impl TomlProject {} +impl DefinedTomlPackage { + fn from_toml_project(project: TomlProject, ws: Option<&TomlWorkspace>) -> CargoResult { + let version = ws_default(Some(project.version), ws, |ws| &ws.version, "version")? + .ok_or_else(|| anyhow!("no version specified"))?; + let edition = ws_default(project.edition, ws, |ws| &ws.edition, "edition")?; + let authors = ws_default(project.authors, ws, |ws| &ws.authors, "authors")?; + let publish = ws_default(project.publish, ws, |ws| &ws.publish, "publish")?; + let description = ws_default(project.description, ws, |ws| &ws.description, "description")?; + let homepage = ws_default(project.homepage, ws, |ws| &ws.homepage, "homepage")?; + let documentation = ws_default( + project.documentation, + ws, + |ws| &ws.documentation, + "documentation", + )?; + let readme = ws_default(project.readme, ws, |ws| &ws.readme, "readme")?; + let keywords = ws_default(project.keywords, ws, |ws| &ws.keywords, "keywords")?; + let categories = ws_default(project.categories, ws, |ws| &ws.categories, "categories")?; + let license = ws_default(project.license, ws, |ws| &ws.license, "license")?; + let license_file = ws_default( + project.license_file, + ws, + |ws| &ws.license_file, + "license_file", + )?; + let repository = ws_default(project.repository, ws, |ws| &ws.repository, "repository")?; -#[derive(Debug, Deserialize, Serialize)] + Ok(Self { + version, + edition, + name: project.name, + authors, + build: project.build, + metabuild: project.metabuild, + links: project.links, + exclude: project.exclude, + include: project.include, + publish, + publish_lockfile: project.publish_lockfile, + workspace: project.workspace, + im_a_teapot: project.im_a_teapot, + autobins: project.autobins, + autoexamples: project.autoexamples, + autotests: project.autotests, + autobenches: project.autobenches, + namespaced_features: project.namespaced_features, + default_run: project.default_run, + + // Package metadata. + description, + homepage, + documentation, + readme, + keywords, + categories, + license, + license_file, + repository, + metadata: project.metadata, + resolver: project.resolver, + }) + } +} + +#[derive(Clone, Debug, Deserialize, Serialize)] pub struct TomlWorkspace { pub members: Option>, #[serde(rename = "default-members")] @@ -1080,31 +1256,15 @@ struct Context<'a, 'b> { features: &'a Features, } -impl TomlManifest { +impl DefinedTomlManifest { pub fn prepare_for_publish( &self, ws: &Workspace<'_>, manifest_file: &Path, - ) -> CargoResult { + ) -> CargoResult { let package_root = manifest_file.parent().unwrap(); let config = ws.config(); - let mut package = self.package().unwrap().clone(); - - let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; - let ws_fields = self.workspace_fields(&parse_output)?; - package.version = MaybeWorkspace::Defined(ws_fields.version.clone()); - package.authors = MaybeWorkspace::from_option(&ws_fields.authors); - package.description = MaybeWorkspace::from_option(&ws_fields.description); - package.documentation = MaybeWorkspace::from_option(&ws_fields.documentation); - package.readme = MaybeWorkspace::from_option(&ws_fields.readme); - package.homepage = MaybeWorkspace::from_option(&ws_fields.homepage); - package.repository = MaybeWorkspace::from_option(&ws_fields.repository); - package.license = MaybeWorkspace::from_option(&ws_fields.license); - package.license_file = MaybeWorkspace::from_option(&ws_fields.license_file); - package.keywords = MaybeWorkspace::from_option(&ws_fields.keywords); - package.categories = MaybeWorkspace::from_option(&ws_fields.categories); - package.publish = MaybeWorkspace::from_option(&ws_fields.publish); - package.edition = MaybeWorkspace::from_option(&ws_fields.edition); + let mut package = self.package.clone().unwrap(); package.workspace = None; let mut cargo_features = self.cargo_features.clone(); @@ -1121,26 +1281,25 @@ impl TomlManifest { } } - if let Some(license_file) = ws_fields.license_file { + if let Some(license_file) = &package.license_file { let license_path = Path::new(&license_file); let abs_license_path = paths::normalize_path(&package_root.join(license_path)); if abs_license_path.strip_prefix(package_root).is_err() { // This path points outside of the package root. `cargo package` // will copy it into the root, so adjust the path to this location. - package.license_file = Some(MaybeWorkspace::Defined( + package.license_file = Some( license_path .file_name() .unwrap() .to_str() .unwrap() .to_string(), - )); + ); } } let all = |_d: &TomlDependency| true; - return Ok(TomlManifest { + Ok(DefinedTomlManifest { package: Some(package), - project: None, profile: self.profile.clone(), lib: self.lib.clone(), bin: self.bin.clone(), @@ -1150,20 +1309,10 @@ impl TomlManifest { dependencies: map_deps(config, self.dependencies.as_ref(), all)?, dev_dependencies: map_deps( config, - self.dev_dependencies - .as_ref() - .or_else(|| self.dev_dependencies2.as_ref()), + self.dev_dependencies.as_ref(), TomlDependency::is_version_specified, )?, - dev_dependencies2: None, - build_dependencies: map_deps( - config, - self.build_dependencies - .as_ref() - .or_else(|| self.build_dependencies2.as_ref()), - all, - )?, - build_dependencies2: None, + build_dependencies: map_deps(config, self.build_dependencies.as_ref(), all)?, features: self.features.clone(), target: match self.target.as_ref().map(|target_map| { target_map @@ -1201,17 +1350,18 @@ impl TomlManifest { replace: None, patch: None, workspace: None, - badges: MaybeWorkspace::from_option(&ws_fields.badges), + badges: self.badges.clone(), cargo_features, - }); + }) } - pub fn to_real_manifest( - me: &Rc, + pub fn into_real_manifest( + self, source_id: SourceId, manifest_file: &Path, config: &Config, ) -> CargoResult<(Manifest, Vec)> { + let me = Rc::new(self); let package_root = manifest_file.parent().unwrap(); let mut nested_paths = vec![]; let mut warnings = vec![]; @@ -1220,11 +1370,9 @@ impl TomlManifest { // Parse features first so they will be available when parsing other parts of the TOML. let empty = Vec::new(); let cargo_features = me.cargo_features.as_ref().unwrap_or(&empty); - let features = Features::new(cargo_features, &mut warnings)?; + let features = Features::new(&cargo_features, &mut warnings)?; - let project = me.package()?; - let parse_output = find_and_parse_workspace_root(manifest_file, &config)?; - let ws_fields = me.workspace_fields(&parse_output)?; + let project = me.package.as_ref().unwrap(); let package_name = project.name.trim(); if package_name.is_empty() { @@ -1233,9 +1381,9 @@ impl TomlManifest { validate_package_name(package_name, "package name", "")?; - let pkgid = PackageId::new(project.name, ws_fields.version, source_id)?; + let pkgid = PackageId::new(project.name, &project.version, source_id)?; - let edition = if let Some(ref edition) = ws_fields.edition { + let edition = if let Some(ref edition) = project.edition { features .require(Feature::edition()) .chain_err(|| "editions are unstable")?; @@ -1258,6 +1406,7 @@ impl TomlManifest { { features.require(Feature::resolver())?; } + let resolve_behavior = match ( project.resolver.as_ref(), me.workspace.as_ref().and_then(|ws| ws.resolver.as_ref()), @@ -1274,7 +1423,7 @@ impl TomlManifest { // If we have a lib with no path, use the inferred lib or else the package name. let targets = targets( &features, - me, + &me, package_name, package_root, edition, @@ -1344,15 +1493,9 @@ impl TomlManifest { // Collect the dependencies. process_dependencies(&mut cx, me.dependencies.as_ref(), None)?; - let dev_deps = me - .dev_dependencies - .as_ref() - .or_else(|| me.dev_dependencies2.as_ref()); + let dev_deps = me.dev_dependencies.as_ref(); process_dependencies(&mut cx, dev_deps, Some(DepKind::Development))?; - let build_deps = me - .build_dependencies - .as_ref() - .or_else(|| me.build_dependencies2.as_ref()); + let build_deps = me.build_dependencies.as_ref(); process_dependencies(&mut cx, build_deps, Some(DepKind::Build))?; for (name, platform) in me.target.iter().flatten() { @@ -1419,17 +1562,17 @@ impl TomlManifest { )?; let metadata = ManifestMetadata { - description: ws_fields.description.clone(), - homepage: ws_fields.homepage.clone(), - documentation: ws_fields.documentation.clone(), - readme: readme_for_project(package_root, ws_fields.readme), - authors: ws_fields.authors.clone().unwrap_or_default(), - license: ws_fields.license.clone(), - license_file: ws_fields.license_file.clone(), - repository: ws_fields.repository.clone(), - keywords: ws_fields.keywords.clone().unwrap_or_default(), - categories: ws_fields.categories.clone().unwrap_or_default(), - badges: ws_fields.badges.clone().unwrap_or_default(), + description: project.description.clone(), + homepage: project.homepage.clone(), + documentation: project.documentation.clone(), + readme: readme_for_project(package_root, &project), + authors: project.authors.clone().unwrap_or_default(), + license: project.license.clone(), + license_file: project.license_file.clone(), + repository: project.repository.clone(), + keywords: project.keywords.clone().unwrap_or_default(), + categories: project.categories.clone().unwrap_or_default(), + badges: me.badges.clone().unwrap_or_default(), links: project.links.clone(), }; @@ -1440,7 +1583,7 @@ impl TomlManifest { profiles.validate(&features, &mut warnings)?; } - let publish = match ws_fields.publish { + let publish = match project.publish { Some(VecStringOrBool::VecString(ref vecstring)) => Some(vecstring.clone()), Some(VecStringOrBool::Bool(false)) => Some(vec![]), None | Some(VecStringOrBool::Bool(true)) => None, @@ -1498,7 +1641,7 @@ impl TomlManifest { edition, project.im_a_teapot, project.default_run.clone(), - Rc::clone(me), + Rc::clone(&me), project.metabuild.clone().map(|sov| sov.0), resolve_behavior, ); @@ -1521,50 +1664,47 @@ impl TomlManifest { Ok((manifest, nested_paths)) } - fn to_virtual_manifest( - me: &Rc, + fn into_virtual_manifest( + self, source_id: SourceId, manifest_file: &Path, config: &Config, ) -> CargoResult<(VirtualManifest, Vec)> { let root = manifest_file.parent().unwrap(); - if me.project.is_some() { + if self.package.is_some() { bail!("this virtual manifest specifies a [project] section, which is not allowed"); } - if me.package.is_some() { - bail!("this virtual manifest specifies a [package] section, which is not allowed"); - } - if me.lib.is_some() { + if self.lib.is_some() { bail!("this virtual manifest specifies a [lib] section, which is not allowed"); } - if me.bin.is_some() { + if self.bin.is_some() { bail!("this virtual manifest specifies a [[bin]] section, which is not allowed"); } - if me.example.is_some() { + if self.example.is_some() { bail!("this virtual manifest specifies a [[example]] section, which is not allowed"); } - if me.test.is_some() { + if self.test.is_some() { bail!("this virtual manifest specifies a [[test]] section, which is not allowed"); } - if me.bench.is_some() { + if self.bench.is_some() { bail!("this virtual manifest specifies a [[bench]] section, which is not allowed"); } - if me.dependencies.is_some() { + if self.dependencies.is_some() { bail!("this virtual manifest specifies a [dependencies] section, which is not allowed"); } - if me.dev_dependencies.is_some() || me.dev_dependencies2.is_some() { + if self.dev_dependencies.is_some() { bail!("this virtual manifest specifies a [dev-dependencies] section, which is not allowed"); } - if me.build_dependencies.is_some() || me.build_dependencies2.is_some() { + if self.build_dependencies.is_some() { bail!("this virtual manifest specifies a [build-dependencies] section, which is not allowed"); } - if me.features.is_some() { + if self.features.is_some() { bail!("this virtual manifest specifies a [features] section, which is not allowed"); } - if me.target.is_some() { + if self.target.is_some() { bail!("this virtual manifest specifies a [target] section, which is not allowed"); } - if me.badges.is_some() { + if self.badges.is_some() { bail!("this virtual manifest specifies a [badges] section, which is not allowed"); } @@ -1572,7 +1712,7 @@ impl TomlManifest { let mut warnings = Vec::new(); let mut deps = Vec::new(); let empty = Vec::new(); - let cargo_features = me.cargo_features.as_ref().unwrap_or(&empty); + let cargo_features = self.cargo_features.as_ref().unwrap_or(&empty); let features = Features::new(cargo_features, &mut warnings)?; let (replace, patch) = { @@ -1587,27 +1727,27 @@ impl TomlManifest { features: &features, root, }; - (me.replace(&mut cx)?, me.patch(&mut cx)?) + (self.replace(&mut cx)?, self.patch(&mut cx)?) }; - let profiles = me.profile.clone(); + let profiles = self.profile.clone(); if let Some(profiles) = &profiles { profiles.validate(&features, &mut warnings)?; } - if me + if self .workspace .as_ref() .map_or(false, |ws| ws.resolver.is_some()) { features.require(Feature::resolver())?; } - let resolve_behavior = me + let resolve_behavior = self .workspace .as_ref() .and_then(|ws| ws.resolver.as_deref()) .map(|r| ResolveBehavior::from_manifest(r)) .transpose()?; - let workspace_config = me.workspace_config(root, config)?; + let workspace_config = self.workspace_config(root, config)?; if !workspace_config.is_root() { bail!("virtual manifests must be configured with [workspace]"); } @@ -1625,100 +1765,63 @@ impl TomlManifest { )) } - fn workspace_fields(&self, parse_output: &Option) -> CargoResult { - let package = self.package()?; - let workspace = parse_output.as_ref().and_then(|output| output.workspace()); - - Ok(WorkspaceFields { - version: workspace_default( - Some(&package.version), - workspace, - |ws| &ws.version, - "version", - )? - .ok_or_else(|| anyhow!("no version specified"))?, - authors: workspace_default( - package.authors.as_ref(), - workspace, - |ws| &ws.authors, - "authors", - )?, - description: workspace_default( - package.description.as_ref(), - workspace, - |ws| &ws.description, - "description", - )?, - documentation: workspace_default( - package.documentation.as_ref(), - workspace, - |ws| &ws.documentation, - "documentation", - )?, - readme: workspace_default( - package.readme.as_ref(), - workspace, - |ws| &ws.readme, - "readme", - )?, - homepage: workspace_default( - package.homepage.as_ref(), - workspace, - |ws| &ws.homepage, - "homepage", - )?, - repository: workspace_default( - package.repository.as_ref(), - workspace, - |ws| &ws.repository, - "repository", - )?, - license: workspace_default( - package.license.as_ref(), - workspace, - |ws| &ws.license, - "license", - )?, - license_file: workspace_default( - package.license_file.as_ref(), - workspace, - |ws| &ws.license_file, - "license_file", - )?, - keywords: workspace_default( - package.keywords.as_ref(), - workspace, - |ws| &ws.keywords, - "keywords", - )?, - categories: workspace_default( - package.categories.as_ref(), - workspace, - |ws| &ws.categories, - "categories", - )?, - publish: workspace_default( - package.publish.as_ref(), - workspace, - |ws| &ws.publish, - "publish", - )?, - edition: workspace_default( - package.edition.as_ref(), - workspace, - |ws| &ws.edition, - "edition", - )?, - badges: workspace_default(self.badges.as_ref(), workspace, |ws| &ws.badges, "badges")?, - }) - } - - pub fn package(&self) -> CargoResult<&Box> { - Ok(self - .project - .as_ref() - .or_else(|| self.package.as_ref()) - .ok_or_else(|| anyhow!("no `package` section found"))?) + pub fn into_toml_manifest(self) -> TomlManifest { + let project = self.package.unwrap(); + + TomlManifest { + cargo_features: self.cargo_features, + package: Some(Box::new(TomlProject { + edition: MaybeWorkspace::from_option(&project.edition), + name: project.name, + version: MaybeWorkspace::Defined(project.version), + authors: MaybeWorkspace::from_option(&project.authors), + build: project.build, + metabuild: project.metabuild, + links: project.links, + exclude: project.exclude, + include: project.include, + publish: MaybeWorkspace::from_option(&project.publish), + publish_lockfile: project.publish_lockfile, + workspace: None, + im_a_teapot: project.im_a_teapot, + autobins: project.autobins, + autoexamples: project.autoexamples, + autotests: project.autotests, + autobenches: project.autobenches, + namespaced_features: project.namespaced_features, + default_run: project.default_run, + + description: MaybeWorkspace::from_option(&project.description), + homepage: MaybeWorkspace::from_option(&project.homepage), + documentation: MaybeWorkspace::from_option(&project.documentation), + readme: MaybeWorkspace::from_option(&project.readme), + keywords: MaybeWorkspace::from_option(&project.keywords), + categories: MaybeWorkspace::from_option(&project.categories), + license: MaybeWorkspace::from_option(&project.license), + license_file: MaybeWorkspace::from_option(&project.license_file), + repository: MaybeWorkspace::from_option(&project.repository), + metadata: project.metadata, + resolver: project.resolver, + })), + project: None, + profile: self.profile, + lib: self.lib, + bin: self.bin, + example: self.example, + test: self.test, + bench: self.bench, + dependencies: self.dependencies, + dev_dependencies: self.dev_dependencies, + dev_dependencies2: None, + build_dependencies: self.build_dependencies, + build_dependencies2: None, + features: self.features, + target: self.target, + replace: self.replace, + patch: self.patch, + workspace: self.workspace, + badges: MaybeWorkspace::from_option(&self.badges), + } } pub fn workspace_config( @@ -1727,7 +1830,7 @@ impl TomlManifest { config: &Config, ) -> CargoResult { let workspace = self.workspace.as_ref(); - let project_workspace = self.package().ok().and_then(|p| p.workspace.as_ref()); + let project_workspace = self.package.as_ref().and_then(|p| p.workspace.as_ref()); Ok(match (workspace, project_workspace) { (Some(toml_workspace), None) => WorkspaceConfig::Root( @@ -1836,8 +1939,8 @@ impl TomlManifest { } /// Returns the name of the README file for a `TomlProject`. -fn readme_for_project(package_root: &Path, readme: Option) -> Option { - match readme { +fn readme_for_project(package_root: &Path, project: &DefinedTomlPackage) -> Option { + match &project.readme { None => default_readme_from_package_root(package_root), Some(value) => match value { StringOrBool::Bool(false) => None, @@ -2143,7 +2246,7 @@ impl ser::Serialize for PathValue { } /// Corresponds to a `target` entry, but `TomlTarget` is already used. -#[derive(Serialize, Deserialize, Debug)] +#[derive(Clone, Serialize, Deserialize, Debug)] struct TomlPlatform { dependencies: Option>, #[serde(rename = "build-dependencies")] diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index a711acd556b..6d34a194da7 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -15,8 +15,8 @@ use std::fs::{self, DirEntry}; use std::path::{Path, PathBuf}; use super::{ - PathValue, StringOrBool, StringOrVec, TomlBenchTarget, TomlBinTarget, TomlExampleTarget, - TomlLibTarget, TomlManifest, TomlTarget, TomlTestTarget, + DefinedTomlManifest, PathValue, StringOrBool, StringOrVec, TomlBenchTarget, TomlBinTarget, + TomlExampleTarget, TomlLibTarget, TomlTarget, TomlTestTarget, }; use crate::core::compiler::CrateType; use crate::core::{Edition, Feature, Features, Target}; @@ -25,7 +25,7 @@ use crate::util::restricted_names; pub fn targets( features: &Features, - manifest: &TomlManifest, + manifest: &DefinedTomlManifest, package_name: &str, package_root: &Path, edition: Edition, @@ -55,7 +55,6 @@ pub fn targets( let package = manifest .package .as_ref() - .or_else(|| manifest.project.as_ref()) .ok_or_else(|| anyhow::format_err!("manifest has no `package` (or `project`)"))?; targets.extend(clean_bins( From 9f504acec41b8d0606a79c7d8ebf881f6a1d2e7c Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 6 Sep 2020 13:01:35 +0100 Subject: [PATCH 11/18] Simplify `MaybeWorkspace` --- src/cargo/util/toml/mod.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index e05b35ba21a..0bb2e87f11a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -895,7 +895,7 @@ fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult { - Workspace(bool), + Workspace, Defined(T), } @@ -973,7 +973,7 @@ where let mvd = de::value::MapAccessDeserializer::new(map); TomlWorkspaceField::deserialize(mvd).and_then(|t| { if t.workspace { - Ok(MaybeWorkspace::Workspace(true)) + Ok(MaybeWorkspace::Workspace) } else { Err(de::Error::custom("workspace cannot be false")) } @@ -1005,7 +1005,7 @@ where Ok(None) => Ok(None), Ok(Some(MaybeWorkspaceBadge::Defined(badges))) => Ok(Some(MaybeWorkspace::Defined(badges))), Ok(Some(MaybeWorkspaceBadge::Workspace(ws))) if ws.workspace => { - Ok(Some(MaybeWorkspace::Workspace(true))) + Ok(Some(MaybeWorkspace::Workspace)) } Ok(Some(MaybeWorkspaceBadge::Workspace(_))) => { Err(de::Error::custom("workspace cannot be false")) @@ -1048,11 +1048,7 @@ where match (value, workspace) { (None, _) => Ok(None), (Some(MaybeWorkspace::Defined(value)), _) => Ok(Some(value)), - (Some(MaybeWorkspace::Workspace(false)), _) => Err(anyhow!( - "error reading {}: cannot specify {{ workspace = false }}", - label - )), - (Some(MaybeWorkspace::Workspace(true)), Some(ws)) => f(ws) + (Some(MaybeWorkspace::Workspace), Some(ws)) => f(ws) .clone() .ok_or_else(|| { anyhow!( @@ -1062,7 +1058,7 @@ where }) .map(|value| Some(value)), - (Some(MaybeWorkspace::Workspace(true)), None) => Err(anyhow!( + (Some(MaybeWorkspace::Workspace), None) => Err(anyhow!( "error reading {}: could not read workspace root", label )), From 6c5af170593224fcaa26d9f0992491117bcd73ea Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 6 Sep 2020 17:11:34 +0100 Subject: [PATCH 12/18] Resolve `readme` and `license-file` relative to the manifest. This will need more tests once we determine the desired behavior. --- src/cargo/util/toml/mod.rs | 97 +++++++++++++++++------- tests/testsuite/deduplicate_workspace.rs | 8 +- 2 files changed, 75 insertions(+), 30 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 0bb2e87f11a..49843ac295a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -310,8 +310,11 @@ impl DefinedTomlManifest { manifest_file: &Path, config: &Config, ) -> CargoResult { - let output = find_workspace_root(manifest_file, config)? - .map(|root_path| parse_manifest(&root_path, config)) + let package_root = manifest_file.parent().unwrap(); + let root_path = find_workspace_root(manifest_file, config)?; + let output = root_path + .as_ref() + .map(|root_path| parse_manifest(root_path, config)) .transpose()?; let workspace = output.as_ref().and_then(|output| output.workspace()); @@ -320,7 +323,7 @@ impl DefinedTomlManifest { .package .or(manifest.project) .map(|p| *p) - .map(|p| DefinedTomlPackage::from_toml_project(p, workspace)) + .map(|p| DefinedTomlPackage::from_toml_project(p, workspace, root_path, package_root)) .transpose()?; let badges = ws_default(manifest.badges, workspace, |ws| &ws.badges, "badges")?; @@ -776,6 +779,16 @@ pub enum StringOrBool { Bool(bool), } +impl StringOrBool { + fn string_or_default(&self, default_value: &str) -> Option { + match self { + Self::String(value) => Some(String::from(value)), + Self::Bool(true) => Some(String::from(default_value)), + Self::Bool(false) => None, + } + } +} + impl<'de> de::Deserialize<'de> for StringOrBool { fn deserialize(deserializer: D) -> Result where @@ -1138,7 +1151,7 @@ struct DefinedTomlPackage { description: Option, homepage: Option, documentation: Option, - readme: Option, + readme: Option, keywords: Option>, categories: Option>, license: Option, @@ -1149,7 +1162,12 @@ struct DefinedTomlPackage { } impl DefinedTomlPackage { - fn from_toml_project(project: TomlProject, ws: Option<&TomlWorkspace>) -> CargoResult { + fn from_toml_project( + project: TomlProject, + ws: Option<&TomlWorkspace>, + root_path: Option, + package_root: &Path, + ) -> CargoResult { let version = ws_default(Some(project.version), ws, |ws| &ws.version, "version")? .ok_or_else(|| anyhow!("no version specified"))?; let edition = ws_default(project.edition, ws, |ws| &ws.edition, "edition")?; @@ -1163,16 +1181,55 @@ impl DefinedTomlPackage { |ws| &ws.documentation, "documentation", )?; - let readme = ws_default(project.readme, ws, |ws| &ws.readme, "readme")?; + + let readme = match (project.readme, ws.and_then(|ws| ws.readme.as_ref())) { + (None, _) => default_readme_from_package_root(package_root), + (Some(MaybeWorkspace::Defined(defined)), _) => defined.string_or_default("README.md"), + (Some(MaybeWorkspace::Workspace), None) => { + bail!("error reading readme: workspace root does not defined [workspace.readme]") + } + (Some(MaybeWorkspace::Workspace), Some(defined)) => { + match defined.string_or_default("README.md") { + Some(ws_readme) => Some( + root_path + .clone() + .unwrap() + .parent() + .unwrap() + .join(ws_readme) + .into_os_string() + .into_string() + .map_err(|_| anyhow!("could not convert readme path into String"))?, + ), + None => None, + } + } + }; + + let license_file = match ( + project.license_file, + ws.and_then(|ws| ws.license_file.as_ref()), + ) { + (None, _) => None, + (Some(MaybeWorkspace::Defined(defined)), _) => Some(defined), + (Some(MaybeWorkspace::Workspace), None) => { + bail!("error reading license-file: workspace root does not defined [workspace.license-file]"); + } + (Some(MaybeWorkspace::Workspace), Some(ws_license_file)) => Some( + root_path + .unwrap() + .parent() + .unwrap() + .join(ws_license_file) + .into_os_string() + .into_string() + .map_err(|_| anyhow!("could not convert license-file path into `String`"))?, + ), + }; + let keywords = ws_default(project.keywords, ws, |ws| &ws.keywords, "keywords")?; let categories = ws_default(project.categories, ws, |ws| &ws.categories, "categories")?; let license = ws_default(project.license, ws, |ws| &ws.license, "license")?; - let license_file = ws_default( - project.license_file, - ws, - |ws| &ws.license_file, - "license_file", - )?; let repository = ws_default(project.repository, ws, |ws| &ws.repository, "repository")?; Ok(Self { @@ -1561,7 +1618,7 @@ impl DefinedTomlManifest { description: project.description.clone(), homepage: project.homepage.clone(), documentation: project.documentation.clone(), - readme: readme_for_project(package_root, &project), + readme: project.readme.clone(), authors: project.authors.clone().unwrap_or_default(), license: project.license.clone(), license_file: project.license_file.clone(), @@ -1790,7 +1847,7 @@ impl DefinedTomlManifest { description: MaybeWorkspace::from_option(&project.description), homepage: MaybeWorkspace::from_option(&project.homepage), documentation: MaybeWorkspace::from_option(&project.documentation), - readme: MaybeWorkspace::from_option(&project.readme), + readme: MaybeWorkspace::from_option(&project.readme.map(StringOrBool::String)), keywords: MaybeWorkspace::from_option(&project.keywords), categories: MaybeWorkspace::from_option(&project.categories), license: MaybeWorkspace::from_option(&project.license), @@ -1934,18 +1991,6 @@ impl DefinedTomlManifest { } } -/// Returns the name of the README file for a `TomlProject`. -fn readme_for_project(package_root: &Path, project: &DefinedTomlPackage) -> Option { - match &project.readme { - None => default_readme_from_package_root(package_root), - Some(value) => match value { - StringOrBool::Bool(false) => None, - StringOrBool::Bool(true) => Some("README.md".to_string()), - StringOrBool::String(v) => Some(v.clone()), - }, - } -} - const DEFAULT_README_FILES: [&str; 3] = ["README.md", "README.txt", "README"]; /// Checks if a file with any of the default README file names exists in the package root. diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs index 2aa0e969c00..3290600bfe7 100644 --- a/tests/testsuite/deduplicate_workspace.rs +++ b/tests/testsuite/deduplicate_workspace.rs @@ -134,8 +134,8 @@ fn inherit_workspace_fields() { edition = { workspace = true } "#, ) - .file("bar/LICENSE", "license") - .file("bar/README.md", "README.md") + .file("LICENSE", "license") + .file("README.md", "README.md") .file("bar/src/main.rs", "fn main() {}") .build(); @@ -155,11 +155,11 @@ fn inherit_workspace_fields() { "homepage": "https://www.rust-lang.org", "keywords": ["cli"], "license": "MIT", - "license_file": "./LICENSE", + "license_file": "../LICENSE", "links": null, "name": "bar", "readme": "README.md", - "readme_file": "README.md", + "readme_file": "../README.md", "repository": "https://github.com/example/example", "vers": "1.2.3" } From 7b80f5b562ded0b64ecf4eb5fdea1a785eed07d6 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Sun, 6 Sep 2020 19:21:14 +0100 Subject: [PATCH 13/18] Allow { workspace = true } when specifying dependencies. --- src/cargo/core/workspace.rs | 6 +- src/cargo/util/toml/mod.rs | 499 +++++++++++++++++++++++++++++------- 2 files changed, 404 insertions(+), 101 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index ab539266e23..b82c4d2f9f6 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -21,7 +21,7 @@ use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; use crate::util::interning::InternedString; use crate::util::paths; use crate::util::toml::{ - map_deps, parse_manifest, read_manifest, ParseOutput, StringOrBool, TomlDependency, + map_deps, parse_manifest, read_manifest, DefinedTomlDependency, ParseOutput, StringOrBool, TomlProfiles, TomlWorkspace, VecStringOrBool, }; use crate::util::{Config, Filesystem}; @@ -145,7 +145,7 @@ pub struct WorkspaceRootConfig { custom_metadata: Option, // Properties that can be inherited by members. - dependencies: Option>, + dependencies: Option>, version: Option, authors: Option>, description: Option, @@ -1149,7 +1149,7 @@ impl WorkspaceRootConfig { let dependencies = map_deps( config, toml_workspace.dependencies.as_ref(), - |_d: &TomlDependency| true, + |_d: &DefinedTomlDependency| true, )?; Ok(Self { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 49843ac295a..7471dbab5b6 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -157,13 +157,69 @@ type TomlExampleTarget = TomlTarget; type TomlTestTarget = TomlTarget; type TomlBenchTarget = TomlTarget; +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(untagged, try_from = "TomlDependency")] +pub enum DefinedTomlDependency { + Simple(String), + Detailed(TomlDependencyDetails), +} + #[derive(Clone, Debug, Serialize)] #[serde(untagged)] pub enum TomlDependency { Simple(String), - Detailed(DetailedTomlDependency), + Workspace(WorkspaceDetails), + Detailed(TomlDependencyDetails), +} + +#[derive(Clone, Debug, Serialize)] +#[serde(into = "TomlWorkspaceDetails")] +pub struct WorkspaceDetails { + features: Option>, + optional: Option, +} + +#[derive(Deserialize, Serialize, Clone, Debug)] +pub struct TomlWorkspaceDetails { + workspace: bool, + features: Option>, + optional: Option, +} + +#[derive(Debug, Deserialize)] +#[serde(untagged)] +enum DefinedTomlDependencyWrapper { + Simple(String), + Workspace(TomlWorkspaceDetails), + Detailed(TomlDependencyDetails), +} + +impl From for TomlWorkspaceDetails { + fn from(workspace_details: WorkspaceDetails) -> Self { + Self { + workspace: true, + features: workspace_details.features, + optional: workspace_details.optional, + } + } } +impl std::convert::TryFrom for DefinedTomlDependency { + type Error = anyhow::Error; + + fn try_from(toml_workspace_dependency: TomlDependency) -> CargoResult { + match toml_workspace_dependency { + TomlDependency::Simple(simple) => Ok(Self::Simple(simple)), + TomlDependency::Detailed(details) => Ok(Self::Detailed(details)), + TomlDependency::Workspace(_) => Err(anyhow!("cannot specify workspace dependency")), + } + } +} + +// This implementation of `Deserialize`, along with many others in this file, exist entirely to +// provide error handling. The error message in derived implementations of `Deserialize` are +// sometimes too opaque to expose to end users. These implementations should otherwise behave +// exactly like their derived counterparts. impl<'de> de::Deserialize<'de> for TomlDependency { fn deserialize(deserializer: D) -> Result where @@ -185,7 +241,7 @@ impl<'de> de::Deserialize<'de> for TomlDependency { where E: de::Error, { - Ok(TomlDependency::Simple(s.to_owned())) + Ok(Self::Value::Simple(s.to_owned())) } fn visit_map(self, map: V) -> Result @@ -193,7 +249,25 @@ impl<'de> de::Deserialize<'de> for TomlDependency { V: de::MapAccess<'de>, { let mvd = de::value::MapAccessDeserializer::new(map); - DetailedTomlDependency::deserialize(mvd).map(TomlDependency::Detailed) + + Ok(match DefinedTomlDependencyWrapper::deserialize(mvd) { + Ok(DefinedTomlDependencyWrapper::Simple(version)) => Self::Value::Simple(version), + Ok(DefinedTomlDependencyWrapper::Detailed(details)) => Self::Value::Detailed(details), + Ok(DefinedTomlDependencyWrapper::Workspace(ws)) if ws.workspace => { + Self::Value::Workspace(WorkspaceDetails { + features: ws.features, + optional: ws.optional, + }) + } + + Ok(DefinedTomlDependencyWrapper::Workspace(_)) => { + return Err(de::Error::custom("workspace cannot be false")); + } + + Err(_) => return Err(de::Error::custom( + "a version string like \"0.9.8\" or a detailed dependency like { version = \"0.9.8\" }", + )), + }) } } @@ -203,7 +277,7 @@ impl<'de> de::Deserialize<'de> for TomlDependency { #[derive(Deserialize, Serialize, Clone, Debug, Default)] #[serde(rename_all = "kebab-case")] -pub struct DetailedTomlDependency { +pub struct TomlDependencyDetails { version: Option, registry: Option, /// The URL of the `registry` field. @@ -248,8 +322,8 @@ pub struct TomlManifest { build_dependencies2: Option>, features: Option>>, target: Option>, - replace: Option>, - patch: Option>>, + replace: Option>, + patch: Option>>, workspace: Option, #[serde(deserialize_with = "deserialize_workspace_badges", default)] badges: Option>>>, @@ -293,13 +367,13 @@ pub struct DefinedTomlManifest { example: Option>, test: Option>, bench: Option>, - dependencies: Option>, - dev_dependencies: Option>, - build_dependencies: Option>, + dependencies: Option>, + dev_dependencies: Option>, + build_dependencies: Option>, features: Option>>, - target: Option>, - replace: Option>, - patch: Option>>, + target: Option>, + replace: Option>, + patch: Option>>, workspace: Option, badges: Option>>, } @@ -312,8 +386,9 @@ impl DefinedTomlManifest { ) -> CargoResult { let package_root = manifest_file.parent().unwrap(); let root_path = find_workspace_root(manifest_file, config)?; + let root_path = root_path.as_deref(); + let output = root_path - .as_ref() .map(|root_path| parse_manifest(root_path, config)) .transpose()?; @@ -328,6 +403,29 @@ impl DefinedTomlManifest { let badges = ws_default(manifest.badges, workspace, |ws| &ws.badges, "badges")?; + let ws_deps = workspace.map(|ws| ws.dependencies.as_ref()).flatten(); + let dependencies = + to_defined_dependencies(manifest.dependencies.as_ref(), ws_deps, root_path)?; + let dev_dependencies = to_defined_dependencies( + manifest + .dev_dependencies + .or(manifest.dev_dependencies2) + .as_ref(), + ws_deps, + root_path, + )?; + + let build_dependencies = to_defined_dependencies( + manifest + .build_dependencies + .or(manifest.build_dependencies2) + .as_ref(), + ws_deps, + root_path, + )?; + + let target = to_defined_platform(manifest.target, ws_deps, root_path)?; + Ok(Self { cargo_features: manifest.cargo_features, package, @@ -337,11 +435,11 @@ impl DefinedTomlManifest { example: manifest.example, test: manifest.test, bench: manifest.bench, - dependencies: manifest.dependencies, - dev_dependencies: manifest.dev_dependencies, - build_dependencies: manifest.build_dependencies, + dependencies, + dev_dependencies, + build_dependencies, features: manifest.features, - target: manifest.target, + target, replace: manifest.replace, patch: manifest.patch, workspace: manifest.workspace, @@ -350,6 +448,110 @@ impl DefinedTomlManifest { } } +fn to_defined_dependencies( + dependencies: Option<&BTreeMap>, + ws_dependencies: Option<&BTreeMap>, + root_path: Option<&Path>, +) -> CargoResult>> { + let empty = BTreeMap::new(); + let ws_deps = ws_dependencies.unwrap_or(&empty); + + map_btree(dependencies, |key, dep| { + DefinedTomlDependency::from_toml_dependency(dep, &key, &ws_deps, root_path) + }) +} + +fn to_toml_dependencies( + dependencies: Option<&BTreeMap>, +) -> Option> { + map_btree(dependencies, |_key, dep| { + Ok(TomlDependency::from_defined_dependency(dep)) + }) + .unwrap() +} + +fn to_defined_platform( + toml_platform: Option>, + ws_dependencies: Option<&BTreeMap>, + root_path: Option<&Path>, +) -> CargoResult>> { + let empty = BTreeMap::new(); + let ws_deps = ws_dependencies.unwrap_or(&empty); + map_btree(toml_platform.as_ref(), |_key, toml_platform| { + DefinedTomlPlatform::from_toml_platform(toml_platform, ws_deps, root_path) + }) +} + +fn to_toml_platform( + defined_platform: Option>, +) -> Option> { + map_btree(defined_platform.as_ref(), |_key, defined_platform| { + Ok(TomlPlatform::from_defined_platform(defined_platform)) + }) + .unwrap() +} + +pub fn map_deps( + config: &Config, + deps: Option<&BTreeMap>, + filter: impl Fn(&DefinedTomlDependency) -> bool, +) -> CargoResult>> { + let deps = match deps { + Some(deps) => deps, + None => return Ok(None), + }; + let deps = deps + .iter() + .filter(|(_k, v)| filter(v)) + .map(|(k, v)| Ok((k.clone(), map_dependency(config, v)?))) + .collect::>>()?; + Ok(Some(deps)) +} + +fn map_dependency( + config: &Config, + dep: &DefinedTomlDependency, +) -> CargoResult { + match dep { + DefinedTomlDependency::Detailed(d) => { + let mut d = d.clone(); + // Path dependencies become crates.io deps. + d.path.take(); + // Same with git dependencies. + d.git.take(); + d.branch.take(); + d.tag.take(); + d.rev.take(); + // registry specifications are elaborated to the index URL + if let Some(registry) = d.registry.take() { + let src = SourceId::alt_registry(config, ®istry)?; + d.registry_index = Some(src.url().to_string()); + } + Ok(DefinedTomlDependency::Detailed(d)) + } + DefinedTomlDependency::Simple(s) => { + Ok(DefinedTomlDependency::Detailed(TomlDependencyDetails { + version: Some(s.clone()), + ..Default::default() + })) + } + } +} + +fn map_btree( + tree: Option<&BTreeMap>, + f: impl Fn(&str, &T) -> CargoResult, +) -> CargoResult>> { + match tree { + None => Ok(None), + Some(deps) => Ok(Some( + deps.iter() + .map(|(key, val)| Ok((key.clone(), f(&*key, val)?))) + .collect::>>()?, + )), + } +} + #[derive(Deserialize, Serialize, Clone, Debug, Default)] pub struct TomlProfiles(BTreeMap); @@ -863,48 +1065,6 @@ impl<'de> de::Deserialize<'de> for VecStringOrBool { } } -pub fn map_deps( - config: &Config, - deps: Option<&BTreeMap>, - filter: impl Fn(&TomlDependency) -> bool, -) -> CargoResult>> { - let deps = match deps { - Some(deps) => deps, - None => return Ok(None), - }; - let deps = deps - .iter() - .filter(|(_k, v)| filter(v)) - .map(|(k, v)| Ok((k.clone(), map_dependency(config, v)?))) - .collect::>>()?; - Ok(Some(deps)) -} - -fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult { - match dep { - TomlDependency::Detailed(d) => { - let mut d = d.clone(); - // Path dependencies become crates.io deps. - d.path.take(); - // Same with git dependencies. - d.git.take(); - d.branch.take(); - d.tag.take(); - d.rev.take(); - // registry specifications are elaborated to the index URL - if let Some(registry) = d.registry.take() { - let src = SourceId::alt_registry(config, ®istry)?; - d.registry_index = Some(src.url().to_string()); - } - Ok(TomlDependency::Detailed(d)) - } - TomlDependency::Simple(s) => Ok(TomlDependency::Detailed(DetailedTomlDependency { - version: Some(s.clone()), - ..Default::default() - })), - } -} - #[derive(Serialize, Clone, Debug)] #[serde(untagged)] pub enum MaybeWorkspace { @@ -1165,7 +1325,7 @@ impl DefinedTomlPackage { fn from_toml_project( project: TomlProject, ws: Option<&TomlWorkspace>, - root_path: Option, + root_path: Option<&Path>, package_root: &Path, ) -> CargoResult { let version = ws_default(Some(project.version), ws, |ws| &ws.version, "version")? @@ -1190,17 +1350,7 @@ impl DefinedTomlPackage { } (Some(MaybeWorkspace::Workspace), Some(defined)) => { match defined.string_or_default("README.md") { - Some(ws_readme) => Some( - root_path - .clone() - .unwrap() - .parent() - .unwrap() - .join(ws_readme) - .into_os_string() - .into_string() - .map_err(|_| anyhow!("could not convert readme path into String"))?, - ), + Some(ws_readme) => Some(join_relative_path(root_path, &ws_readme)?), None => None, } } @@ -1215,16 +1365,9 @@ impl DefinedTomlPackage { (Some(MaybeWorkspace::Workspace), None) => { bail!("error reading license-file: workspace root does not defined [workspace.license-file]"); } - (Some(MaybeWorkspace::Workspace), Some(ws_license_file)) => Some( - root_path - .unwrap() - .parent() - .unwrap() - .join(ws_license_file) - .into_os_string() - .into_string() - .map_err(|_| anyhow!("could not convert license-file path into `String`"))?, - ), + (Some(MaybeWorkspace::Workspace), Some(ws_license_file)) => { + Some(join_relative_path(root_path, ws_license_file)?) + } }; let keywords = ws_default(project.keywords, ws, |ws| &ws.keywords, "keywords")?; @@ -1269,6 +1412,17 @@ impl DefinedTomlPackage { } } +fn join_relative_path(root_path: Option<&Path>, relative_path: &str) -> CargoResult { + root_path + .unwrap() + .parent() + .unwrap() + .join(relative_path) + .into_os_string() + .into_string() + .map_err(|_| anyhow!("could not convert path into `String`")) +} + #[derive(Clone, Debug, Deserialize, Serialize)] pub struct TomlWorkspace { pub members: Option>, @@ -1279,7 +1433,7 @@ pub struct TomlWorkspace { resolver: Option, // Properties that can be inherited by members. - pub dependencies: Option>, + pub dependencies: Option>, pub version: Option, pub authors: Option>, pub description: Option, @@ -1350,7 +1504,7 @@ impl DefinedTomlManifest { ); } } - let all = |_d: &TomlDependency| true; + let all = |_d: &DefinedTomlDependency| true; Ok(DefinedTomlManifest { package: Some(package), profile: self.profile.clone(), @@ -1363,7 +1517,7 @@ impl DefinedTomlManifest { dev_dependencies: map_deps( config, self.dev_dependencies.as_ref(), - TomlDependency::is_version_specified, + DefinedTomlDependency::is_version_specified, )?, build_dependencies: map_deps(config, self.build_dependencies.as_ref(), all)?, features: self.features.clone(), @@ -1373,14 +1527,14 @@ impl DefinedTomlManifest { .map(|(k, v)| { Ok(( k.clone(), - TomlPlatform { + DefinedTomlPlatform { dependencies: map_deps(config, v.dependencies.as_ref(), all)?, dev_dependencies: map_deps( config, v.dev_dependencies .as_ref() .or_else(|| v.dev_dependencies2.as_ref()), - TomlDependency::is_version_specified, + DefinedTomlDependency::is_version_specified, )?, dev_dependencies2: None, build_dependencies: map_deps( @@ -1528,7 +1682,7 @@ impl DefinedTomlManifest { fn process_dependencies( cx: &mut Context<'_, '_>, - new_deps: Option<&BTreeMap>, + new_deps: Option<&BTreeMap>, kind: Option, ) -> CargoResult<()> { let dependencies = match new_deps { @@ -1863,13 +2017,13 @@ impl DefinedTomlManifest { example: self.example, test: self.test, bench: self.bench, - dependencies: self.dependencies, - dev_dependencies: self.dev_dependencies, + dependencies: to_toml_dependencies(self.dependencies.as_ref()), + dev_dependencies: to_toml_dependencies(self.dev_dependencies.as_ref()), dev_dependencies2: None, - build_dependencies: self.build_dependencies, + build_dependencies: to_toml_dependencies(self.build_dependencies.as_ref()), build_dependencies2: None, features: self.features, - target: self.target, + target: to_toml_platform(self.target), replace: self.replace, patch: self.patch, workspace: self.workspace, @@ -2021,6 +2175,89 @@ fn unique_build_targets(targets: &[Target], package_root: &Path) -> Result<(), S } impl TomlDependency { + fn from_defined_dependency(dep: &DefinedTomlDependency) -> Self { + match dep { + DefinedTomlDependency::Simple(s) => Self::Simple(s.clone()), + DefinedTomlDependency::Detailed(detailed) => Self::Detailed(detailed.clone()), + } + } +} + +impl DefinedTomlDependency { + fn from_toml_dependency( + dep: &TomlDependency, + name: &str, + ws_deps: &BTreeMap, + root_path: Option<&Path>, + ) -> CargoResult { + match dep { + TomlDependency::Simple(s) => Ok(Self::Simple(s.clone())), + TomlDependency::Detailed(detailed) => Ok(Self::Detailed(detailed.clone())), + TomlDependency::Workspace(ws) => { + let ws_dep = ws_deps.get(name).ok_or_else(|| { + anyhow!( + "could not find entry in [workspace.dependencies] for \"{}\"", + name + ) + })?; + + Ok(Self::from_workspace_dependency(ws, ws_dep, root_path)?) + } + } + } + + fn from_workspace_dependency( + details: &WorkspaceDetails, + ws_dep: &Self, + root_path: Option<&Path>, + ) -> CargoResult { + let details = match ws_dep { + Self::Simple(s) => TomlDependencyDetails { + version: Some(s.clone()), + features: details + .features + .clone() + .or_else(|| ws_dep.features().cloned()), + optional: details.optional.or_else(|| Some(ws_dep.is_optional())), + ..Default::default() + }, + + Self::Detailed(d) => TomlDependencyDetails { + version: d.version.clone(), + registry: d.registry.clone(), + registry_index: d.registry_index.clone(), + path: d + .path + .clone() + .map(|p| join_relative_path(root_path, &p)) + .transpose()?, + git: d.git.clone(), + branch: d.branch.clone(), + tag: d.tag.clone(), + rev: d.rev.clone(), + features: match (&details.features, &d.features) { + (None, None) => None, + (Some(features), None) | (None, Some(features)) => Some(features.clone()), + (Some(ws_features), Some(features)) => { + let mut result = ws_features.clone(); + for f in features { + if !result.contains(&f) { + result.push(f.clone()); + } + } + Some(result) + } + }, + optional: details.optional.or_else(|| d.optional.clone()), + default_features: d.default_features.clone(), + default_features2: d.default_features2.clone(), + package: d.package.clone(), + public: d.public.clone(), + }, + }; + + Ok(Self::Detailed(details)) + } fn to_dependency( &self, name: &str, @@ -2028,31 +2265,38 @@ impl TomlDependency { kind: Option, ) -> CargoResult { match *self { - TomlDependency::Simple(ref version) => DetailedTomlDependency { + DefinedTomlDependency::Simple(ref version) => TomlDependencyDetails { version: Some(version.clone()), ..Default::default() } .to_dependency(name, cx, kind), - TomlDependency::Detailed(ref details) => details.to_dependency(name, cx, kind), + DefinedTomlDependency::Detailed(ref details) => details.to_dependency(name, cx, kind), } } fn is_version_specified(&self) -> bool { match self { - TomlDependency::Detailed(d) => d.version.is_some(), - TomlDependency::Simple(..) => true, + DefinedTomlDependency::Detailed(d) => d.version.is_some(), + DefinedTomlDependency::Simple(..) => true, } } fn is_optional(&self) -> bool { match self { - TomlDependency::Detailed(d) => d.optional.unwrap_or(false), - TomlDependency::Simple(..) => false, + DefinedTomlDependency::Detailed(d) => d.optional.unwrap_or(false), + DefinedTomlDependency::Simple(..) => false, + } + } + + fn features(&self) -> Option<&Vec> { + match self { + DefinedTomlDependency::Detailed(d) => d.features.as_ref(), + DefinedTomlDependency::Simple(..) => None, } } } -impl DetailedTomlDependency { +impl TomlDependencyDetails { fn to_dependency( &self, name_in_toml: &str, @@ -2287,6 +2531,19 @@ impl ser::Serialize for PathValue { } /// Corresponds to a `target` entry, but `TomlTarget` is already used. +#[derive(Clone, Serialize, Deserialize, Debug)] +struct DefinedTomlPlatform { + dependencies: Option>, + #[serde(rename = "build-dependencies")] + build_dependencies: Option>, + #[serde(rename = "build_dependencies")] + build_dependencies2: Option>, + #[serde(rename = "dev-dependencies")] + dev_dependencies: Option>, + #[serde(rename = "dev_dependencies")] + dev_dependencies2: Option>, +} + #[derive(Clone, Serialize, Deserialize, Debug)] struct TomlPlatform { dependencies: Option>, @@ -2300,6 +2557,52 @@ struct TomlPlatform { dev_dependencies2: Option>, } +impl TomlPlatform { + fn from_defined_platform(defined_platform: &DefinedTomlPlatform) -> Self { + Self { + dependencies: to_toml_dependencies(defined_platform.dependencies.as_ref()), + build_dependencies: to_toml_dependencies(defined_platform.build_dependencies.as_ref()), + build_dependencies2: None, + dev_dependencies: to_toml_dependencies(defined_platform.dev_dependencies.as_ref()), + dev_dependencies2: None, + } + } +} + +impl DefinedTomlPlatform { + fn from_toml_platform( + toml_platform: &TomlPlatform, + ws_deps: &BTreeMap, + root_path: Option<&Path>, + ) -> CargoResult { + let build_dependencies = toml_platform + .build_dependencies + .as_ref() + .or(toml_platform.build_dependencies2.as_ref()); + + let dev_dependencies = toml_platform + .dev_dependencies + .as_ref() + .or(toml_platform.dev_dependencies2.as_ref()); + + Ok(Self { + dependencies: to_defined_dependencies( + toml_platform.dependencies.as_ref(), + Some(ws_deps), + root_path, + )?, + build_dependencies: to_defined_dependencies( + build_dependencies, + Some(ws_deps), + root_path, + )?, + build_dependencies2: None, + dev_dependencies: to_defined_dependencies(dev_dependencies, Some(ws_deps), root_path)?, + dev_dependencies2: None, + }) + } +} + impl TomlTarget { fn new() -> TomlTarget { TomlTarget::default() From 4b32feef71a87f2683f926118565a7d1aea8f34a Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Mon, 7 Sep 2020 16:42:59 +0100 Subject: [PATCH 14/18] Add tests for dependency { workspace = true } --- tests/testsuite/deduplicate_workspace.rs | 383 ++++++++++++++++++++++- 1 file changed, 380 insertions(+), 3 deletions(-) diff --git a/tests/testsuite/deduplicate_workspace.rs b/tests/testsuite/deduplicate_workspace.rs index 3290600bfe7..5c97e9b20eb 100644 --- a/tests/testsuite/deduplicate_workspace.rs +++ b/tests/testsuite/deduplicate_workspace.rs @@ -1,5 +1,9 @@ //! Tests for deduplicating Cargo.toml fields with { workspace = true } -use cargo_test_support::{basic_workspace_manifest, git, paths, project, publish, registry}; +use cargo_test_support::registry::{Dependency, Package}; +use cargo_test_support::{ + basic_lib_manifest, basic_manifest, basic_workspace_manifest, git, path2url, paths, project, + publish, registry, +}; #[cargo_test] fn permit_additional_workspace_fields() { @@ -27,7 +31,7 @@ fn permit_additional_workspace_fields() { gitlab = { repository = "https://gitlab.com/rust-lang/rust", branch = "master" } [workspace.dependencies] - dep1 = "0.1" + dep = "0.1" "#, ) .file("bar/Cargo.toml", &basic_workspace_manifest("bar", "..")) @@ -46,7 +50,7 @@ fn permit_additional_workspace_fields() { p.cargo("check").run(); let lockfile = p.read_lockfile(); - assert!(!lockfile.contains("dep1")); + assert!(!lockfile.contains("dep")); } #[cargo_test] @@ -268,6 +272,336 @@ fn inherit_own_workspace_fields() { ); } +#[cargo_test] +fn inherit_dependencies() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + dep = "0.1" + dep-build = "0.8" + dep-dev = "0.5.2" + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [project] + workspace = ".." + name = "bar" + version = "0.2.0" + authors = [] + + [dependencies] + dep = { workspace = true } + + [build-dependencies] + dep-build = { workspace = true } + + [dev-dependencies] + dep-dev = { workspace = true } + + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + Package::new("dep", "0.1.2").publish(); + Package::new("dep-build", "0.8.2").publish(); + Package::new("dep-dev", "0.5.2").publish(); + + p.cargo("build") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] dep-build v0.8.2 ([..]) +[DOWNLOADED] dep v0.1.2 ([..]) +[COMPILING] dep v0.1.2 +[COMPILING] bar v0.2.0 ([CWD]/bar) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + + p.cargo("check").run(); + let lockfile = p.read_lockfile(); + assert!(lockfile.contains("dep")); + assert!(lockfile.contains("dep-dev")); + assert!(lockfile.contains("dep-build")); +} + +#[cargo_test] +fn inherite_detailed_dependencies() { + let git_project = git::new("detailed", |project| { + project + .file("Cargo.toml", &basic_lib_manifest("detailed")) + .file( + "src/detailed.rs", + r#" + pub fn hello() -> &'static str { + "hello world" + } + "#, + ) + }); + + // Make a new branch based on the current HEAD commit + let repo = git2::Repository::open(&git_project.root()).unwrap(); + let head = repo.head().unwrap().target().unwrap(); + let head = repo.find_commit(head).unwrap(); + repo.branch("branchy", &head, true).unwrap(); + + let p = project() + .file( + "Cargo.toml", + &format!( + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + detailed = {{ git = '{}', branch = "branchy" }} + "#, + git_project.url() + ), + ) + .file( + "bar/Cargo.toml", + r#" + [project] + workspace = ".." + name = "bar" + version = "0.2.0" + authors = [] + + [dependencies] + detailed = { workspace = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + let git_root = git_project.root(); + + p.cargo("build") + .with_stderr(&format!( + "\ +[UPDATING] git repository `{}`\n\ +[COMPILING] detailed v0.5.0 ({}?branch=branchy#[..])\n\ +[COMPILING] bar v0.2.0 ([CWD]/bar)\n\ +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\n", + path2url(&git_root), + path2url(&git_root), + )) + .run(); +} + +#[cargo_test] +fn inherit_path_dependencies() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + dep = { path = "dep" } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [project] + workspace = ".." + name = "bar" + version = "0.2.0" + authors = [] + + [dependencies] + dep = { workspace = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .file("dep/Cargo.toml", &basic_manifest("dep", "0.9.0")) + .file("dep/src/lib.rs", "") + .build(); + + p.cargo("build") + .with_stderr( + "\ +[COMPILING] dep v0.9.0 ([CWD]/dep) +[COMPILING] bar v0.2.0 ([CWD]/bar) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!(lockfile.contains("dep")); +} + +#[cargo_test] +fn inherit_target_dependencies() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + dep = "0.1" + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [project] + workspace = ".." + name = "bar" + version = "0.2.0" + authors = [] + + [target.'cfg(unix)'.dependencies] + dep = { workspace = true } + + [target.'cfg(windows)'.dependencies] + dep = { workspace = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + Package::new("dep", "0.1.2").publish(); + + p.cargo("build") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] dep v0.1.2 ([..]) +[COMPILING] dep v0.1.2 +[COMPILING] bar v0.2.0 ([CWD]/bar) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!(lockfile.contains("dep")); +} + +#[cargo_test] +fn inherited_dependencies_union_features() { + Package::new("dep", "0.1.0") + .feature("fancy", &["fancy_dep"]) + .feature("dancy", &["dancy_dep"]) + .add_dep(Dependency::new("fancy_dep", "0.2").optional(true)) + .add_dep(Dependency::new("dancy_dep", "0.6").optional(true)) + .file("src/lib.rs", "") + .publish(); + + Package::new("fancy_dep", "0.2.4").publish(); + Package::new("dancy_dep", "0.6.8").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + dep = { version = "0.1", features = ["fancy"] } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [project] + workspace = ".." + name = "bar" + version = "0.2.0" + authors = [] + + [dependencies] + dep = { workspace = true, features = ["dancy"] } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] fancy_dep v0.2.4 ([..]) +[DOWNLOADED] dep v0.1.0 ([..]) +[DOWNLOADED] dancy_dep v0.6.8 ([..]) +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] dep v0.1.0 +[COMPILING] bar v0.2.0 ([CWD]/bar) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!(lockfile.contains("dep")); + assert!(lockfile.contains("fancy_dep")); + assert!(lockfile.contains("dancy_dep")); +} + +#[cargo_test] +fn inherited_dependency_override_optional() { + Package::new("dep", "0.1.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + + [workspace.dependencies] + dep = "0.1.0" + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [project] + workspace = ".." + name = "bar" + version = "0.2.0" + authors = [] + + [dependencies] + dep = { workspace = true, optional = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .with_stderr( + "\ +[UPDATING] `[..]` index +[COMPILING] bar v0.2.0 ([CWD]/bar) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); +} + #[cargo_test] fn error_inherit_from_undefined_field() { registry::init(); @@ -423,3 +757,46 @@ Caused by: ) .run(); } + +#[cargo_test] +fn error_inherit_unspecified_dependency() { + let p = project().build(); + + let _ = git::repo(&paths::root().join("foo")) + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bar"] + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + workspace = ".." + version = "1.2.3" + authors = ["rustaceans"] + + [dependencies] + foo = { workspace = true } + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .build(); + + p.cargo("build") + .cwd("bar") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + could not find entry in [workspace.dependencies] for \"foo\" +", + ) + .run(); +} From 34152092ae363ba53913e77274c7a3b6d6a73494 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Tue, 8 Sep 2020 02:43:02 +0100 Subject: [PATCH 15/18] Add WorkspaceContext --- src/cargo/util/toml/mod.rs | 69 +++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 7471dbab5b6..db17ab09b74 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -378,6 +378,11 @@ pub struct DefinedTomlManifest { badges: Option>>, } +struct WorkspaceContext<'a> { + dependencies: &'a BTreeMap, + root_path: Option<&'a Path>, +} + impl DefinedTomlManifest { fn from_toml_manifest( manifest: TomlManifest, @@ -394,6 +399,15 @@ impl DefinedTomlManifest { let workspace = output.as_ref().and_then(|output| output.workspace()); + let empty = BTreeMap::new(); + let ctx = WorkspaceContext { + dependencies: workspace + .map(|ws| ws.dependencies.as_ref()) + .flatten() + .unwrap_or(&empty), + root_path, + }; + let package = manifest .package .or(manifest.project) @@ -403,16 +417,13 @@ impl DefinedTomlManifest { let badges = ws_default(manifest.badges, workspace, |ws| &ws.badges, "badges")?; - let ws_deps = workspace.map(|ws| ws.dependencies.as_ref()).flatten(); - let dependencies = - to_defined_dependencies(manifest.dependencies.as_ref(), ws_deps, root_path)?; + let dependencies = to_defined_dependencies(manifest.dependencies.as_ref(), &ctx)?; let dev_dependencies = to_defined_dependencies( manifest .dev_dependencies .or(manifest.dev_dependencies2) .as_ref(), - ws_deps, - root_path, + &ctx, )?; let build_dependencies = to_defined_dependencies( @@ -420,11 +431,10 @@ impl DefinedTomlManifest { .build_dependencies .or(manifest.build_dependencies2) .as_ref(), - ws_deps, - root_path, + &ctx, )?; - let target = to_defined_platform(manifest.target, ws_deps, root_path)?; + let target = to_defined_platform(manifest.target, &ctx)?; Ok(Self { cargo_features: manifest.cargo_features, @@ -450,14 +460,10 @@ impl DefinedTomlManifest { fn to_defined_dependencies( dependencies: Option<&BTreeMap>, - ws_dependencies: Option<&BTreeMap>, - root_path: Option<&Path>, + ctx: &WorkspaceContext<'_>, ) -> CargoResult>> { - let empty = BTreeMap::new(); - let ws_deps = ws_dependencies.unwrap_or(&empty); - map_btree(dependencies, |key, dep| { - DefinedTomlDependency::from_toml_dependency(dep, &key, &ws_deps, root_path) + DefinedTomlDependency::from_toml_dependency(dep, &key, ctx) }) } @@ -472,13 +478,10 @@ fn to_toml_dependencies( fn to_defined_platform( toml_platform: Option>, - ws_dependencies: Option<&BTreeMap>, - root_path: Option<&Path>, + ctx: &WorkspaceContext<'_>, ) -> CargoResult>> { - let empty = BTreeMap::new(); - let ws_deps = ws_dependencies.unwrap_or(&empty); map_btree(toml_platform.as_ref(), |_key, toml_platform| { - DefinedTomlPlatform::from_toml_platform(toml_platform, ws_deps, root_path) + DefinedTomlPlatform::from_toml_platform(toml_platform, ctx) }) } @@ -2187,21 +2190,20 @@ impl DefinedTomlDependency { fn from_toml_dependency( dep: &TomlDependency, name: &str, - ws_deps: &BTreeMap, - root_path: Option<&Path>, + ctx: &WorkspaceContext<'_>, ) -> CargoResult { match dep { TomlDependency::Simple(s) => Ok(Self::Simple(s.clone())), TomlDependency::Detailed(detailed) => Ok(Self::Detailed(detailed.clone())), TomlDependency::Workspace(ws) => { - let ws_dep = ws_deps.get(name).ok_or_else(|| { + let ws_dep = ctx.dependencies.get(name).ok_or_else(|| { anyhow!( "could not find entry in [workspace.dependencies] for \"{}\"", name ) })?; - Ok(Self::from_workspace_dependency(ws, ws_dep, root_path)?) + Ok(Self::from_workspace_dependency(ws, ws_dep, ctx)?) } } } @@ -2209,7 +2211,7 @@ impl DefinedTomlDependency { fn from_workspace_dependency( details: &WorkspaceDetails, ws_dep: &Self, - root_path: Option<&Path>, + ctx: &WorkspaceContext<'_>, ) -> CargoResult { let details = match ws_dep { Self::Simple(s) => TomlDependencyDetails { @@ -2229,7 +2231,7 @@ impl DefinedTomlDependency { path: d .path .clone() - .map(|p| join_relative_path(root_path, &p)) + .map(|p| join_relative_path(ctx.root_path, &p)) .transpose()?, git: d.git.clone(), branch: d.branch.clone(), @@ -2572,8 +2574,7 @@ impl TomlPlatform { impl DefinedTomlPlatform { fn from_toml_platform( toml_platform: &TomlPlatform, - ws_deps: &BTreeMap, - root_path: Option<&Path>, + ctx: &WorkspaceContext<'_>, ) -> CargoResult { let build_dependencies = toml_platform .build_dependencies @@ -2586,18 +2587,10 @@ impl DefinedTomlPlatform { .or(toml_platform.dev_dependencies2.as_ref()); Ok(Self { - dependencies: to_defined_dependencies( - toml_platform.dependencies.as_ref(), - Some(ws_deps), - root_path, - )?, - build_dependencies: to_defined_dependencies( - build_dependencies, - Some(ws_deps), - root_path, - )?, + dependencies: to_defined_dependencies(toml_platform.dependencies.as_ref(), ctx)?, + build_dependencies: to_defined_dependencies(build_dependencies, ctx)?, build_dependencies2: None, - dev_dependencies: to_defined_dependencies(dev_dependencies, Some(ws_deps), root_path)?, + dev_dependencies: to_defined_dependencies(dev_dependencies, ctx)?, dev_dependencies2: None, }) } From 33fe33130beb88737492f6c2fd01ba447c099203 Mon Sep 17 00:00:00 2001 From: Mukund Lakshman Date: Tue, 8 Sep 2020 03:31:44 +0100 Subject: [PATCH 16/18] Infer the version of path dependencies by peeking into their manifests. --- src/cargo/core/workspace.rs | 4 +- src/cargo/ops/cargo_package.rs | 17 ------- src/cargo/ops/registry.rs | 4 +- src/cargo/util/toml/manifest_cache.rs | 9 +++- src/cargo/util/toml/mod.rs | 73 +++++++++++++++++++++------ 5 files changed, 70 insertions(+), 37 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index b82c4d2f9f6..023d93a70f8 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -21,7 +21,7 @@ use crate::util::errors::{CargoResult, CargoResultExt, ManifestError}; use crate::util::interning::InternedString; use crate::util::paths; use crate::util::toml::{ - map_deps, parse_manifest, read_manifest, DefinedTomlDependency, ParseOutput, StringOrBool, + parse_manifest, prepare_deps, read_manifest, DefinedTomlDependency, ParseOutput, StringOrBool, TomlProfiles, TomlWorkspace, VecStringOrBool, }; use crate::util::{Config, Filesystem}; @@ -1146,7 +1146,7 @@ impl WorkspaceRootConfig { config: &Config, toml_workspace: &TomlWorkspace, ) -> CargoResult { - let dependencies = map_deps( + let dependencies = prepare_deps( config, toml_workspace.dependencies.as_ref(), |_d: &DefinedTomlDependency| true, diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 3a2179e3783..356d652176c 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -105,8 +105,6 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult