Skip to content

Commit

Permalink
Auto merge of #10497 - Muscraft:rfc2906-part1, r=epage
Browse files Browse the repository at this point in the history
Part 1 of RFC2906 - Packages can inherit fields from their root workspace

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

All changes were heavily inspired by #8664 and #9684

A big thanks to both `@yaymukund` and `@ParkMyCar.` I pulled a lot of inspiration from their branches.

I would also like to thank `@alexcrichton` for all the feedback on the first attempt. It has brought this into a much better state.

All changes have been made according to the RFC as well as `@alexcrichton's` [comment](#8664 (comment)).

This is part 1 of many, as described by [this comment](#9684 (comment)), [this comment](#9684 (review)) and redefined  [by this one](#10497 (comment)).

This PR focuses on inheriting in root package, including:
- Add `MaybeWorkspace<T>` to allow for `{ workspace = true }`
- Add a new variant to `TomlDependency` to allow inheriting dependencies from a workspace
- Add `InheritableFields` so that we have somewhere to get a value from when we `resolve` a `MaybeWorkspace<T>`
  - `license_file` and `readme` are in `InheritableFields` in this part but are not `MaybeWorkspace` for reasons [described here](#10497 (comment))
- Add a method to `resolve` a `MaybeWorkspace<T>` into a `T` that can fail if we have nowhere to pull from or the workspace does not define that field
- Disallow inheriting from a different `Cargo.toml`
  - This means that you can only inherit from inside the same `Cargo.toml`, i.e. it has both a `[workspace]` and a `[package]`
  - Forcing this requirement allows us to test the implementations of `MaybeWorkspace<T>` and the new `TomlDependency` variant without having to add searching for a workspace root and the complexity with it

Remaining implementation work for the RFC
- Support inheriting in any workspace member
- Correctly inherit fields that are relative paths
  - Including adding support for inheriting `license_file`,  `readme`, and path-dependencies
-  Path dependencies infer version directive
- Lock workspace dependencies and warn when unused
- Optimizations, as needed
- Evaluate any new fields for being inheritable (e.g. `rust-version`)
- Evaluate making users of `{ workspace = true }` define the workspace they pull from in `[package]`

Areas of concern:
- `TomlDependency` Deserialization is a mess, and I could not figure out how to do it in a cleaner way without significant headaches. I ended up having to do the same thing as last time [which was an issue](#9684 (comment)).
- Resolving on a `MaybeWorkspace` feels extremely verbose currently:
  ```rust
  project.homepage.clone().map(|mw| mw.resolve(&features, "homepage", || get_ws(inheritable)?.homepage())).transpose()?,
  ```
  This way allows for lazy resolution of inheritable fields and finding a workspace (in part 2) so you do not pay a penalty for not using this feature. The other bit of good news is `&features` will go away as it is just feature checking currently.

- This feature requires some level of duplication of code as well as remaking the original `TomlManifest` which adds extra length to the changes.
- This feature also takes a linear process and makes it potentially non-linear which adds lots of complexity (which will get worse in Part 2)

Please let me know if you have any feedback or suggestions!
  • Loading branch information
bors committed Mar 25, 2022
2 parents af8ddf5 + e6992d4 commit b1636fc
Show file tree
Hide file tree
Showing 8 changed files with 1,759 additions and 145 deletions.
1 change: 1 addition & 0 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub fn resolve_std<'cfg>(
&Some(members),
/*default_members*/ &None,
/*exclude*/ &None,
/*inheritable*/ &None,
/*custom_metadata*/ &None,
));
let virtual_manifest = crate::core::VirtualManifest::new(
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,9 @@ features! {

// Allow specifying rustflags directly in a profile
(unstable, profile_rustflags, "", "reference/unstable.html#profile-rustflags-option"),

// Allow specifying rustflags directly in a profile
(unstable, workspace_inheritance, "", "reference/unstable.html#workspace-inheritance"),
}

pub struct Feature {
Expand Down
4 changes: 3 additions & 1 deletion src/cargo/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ pub use self::resolver::{Resolve, ResolveVersion};
pub use self::shell::{Shell, Verbosity};
pub use self::source::{GitReference, Source, SourceId, SourceMap};
pub use self::summary::{FeatureMap, FeatureValue, Summary};
pub use self::workspace::{MaybePackage, Workspace, WorkspaceConfig, WorkspaceRootConfig};
pub use self::workspace::{
InheritableFields, MaybePackage, Workspace, WorkspaceConfig, WorkspaceRootConfig,
};

pub mod compiler;
pub mod dependency;
Expand Down
206 changes: 203 additions & 3 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::collections::{BTreeMap, BTreeSet, HashSet};
use std::path::{Path, PathBuf};
use std::rc::Rc;

use anyhow::{bail, Context as _};
use anyhow::{anyhow, bail, Context as _};
use glob::glob;
use itertools::Itertools;
use log::debug;
Expand All @@ -22,7 +22,9 @@ use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
use crate::util::errors::{CargoResult, ManifestError};
use crate::util::interning::InternedString;
use crate::util::lev_distance;
use crate::util::toml::{read_manifest, TomlDependency, TomlProfiles};
use crate::util::toml::{
read_manifest, StringOrBool, TomlDependency, TomlProfiles, VecStringOrBool,
};
use crate::util::{config::ConfigRelativePath, Config, Filesystem, IntoUrl};
use cargo_util::paths;

Expand Down Expand Up @@ -123,6 +125,15 @@ pub enum WorkspaceConfig {
Member { root: Option<String> },
}

impl WorkspaceConfig {
pub fn inheritable(&self) -> Option<&InheritableFields> {
match self {
WorkspaceConfig::Root(root) => Some(&root.inheritable_fields),
WorkspaceConfig::Member { .. } => None,
}
}
}

/// 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
Expand All @@ -133,6 +144,7 @@ pub struct WorkspaceRootConfig {
members: Option<Vec<String>>,
default_members: Option<Vec<String>>,
exclude: Vec<String>,
inheritable_fields: InheritableFields,
custom_metadata: Option<toml::Value>,
}

Expand Down Expand Up @@ -1567,17 +1579,18 @@ impl WorkspaceRootConfig {
members: &Option<Vec<String>>,
default_members: &Option<Vec<String>>,
exclude: &Option<Vec<String>>,
inheritable: &Option<InheritableFields>,
custom_metadata: &Option<toml::Value>,
) -> WorkspaceRootConfig {
WorkspaceRootConfig {
root_dir: root_dir.to_path_buf(),
members: members.clone(),
default_members: default_members.clone(),
exclude: exclude.clone().unwrap_or_default(),
inheritable_fields: inheritable.clone().unwrap_or_default(),
custom_metadata: custom_metadata.clone(),
}
}

/// Checks the path against the `excluded` list.
///
/// This method does **not** consider the `members` list.
Expand Down Expand Up @@ -1641,3 +1654,190 @@ impl WorkspaceRootConfig {
Ok(res)
}
}

/// A group of fields that are inheritable by members of the workspace
#[derive(Clone, Debug, Default)]
pub struct InheritableFields {
dependencies: Option<BTreeMap<String, TomlDependency>>,
version: Option<semver::Version>,
authors: Option<Vec<String>>,
description: Option<String>,
homepage: Option<String>,
documentation: Option<String>,
readme: Option<StringOrBool>,
keywords: Option<Vec<String>>,
categories: Option<Vec<String>>,
license: Option<String>,
license_file: Option<String>,
repository: Option<String>,
publish: Option<VecStringOrBool>,
edition: Option<String>,
badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
}

impl InheritableFields {
pub fn new(
dependencies: Option<BTreeMap<String, TomlDependency>>,
version: Option<semver::Version>,
authors: Option<Vec<String>>,
description: Option<String>,
homepage: Option<String>,
documentation: Option<String>,
readme: Option<StringOrBool>,
keywords: Option<Vec<String>>,
categories: Option<Vec<String>>,
license: Option<String>,
license_file: Option<String>,
repository: Option<String>,
publish: Option<VecStringOrBool>,
edition: Option<String>,
badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
) -> InheritableFields {
Self {
dependencies,
version,
authors,
description,
homepage,
documentation,
readme,
keywords,
categories,
license,
license_file,
repository,
publish,
edition,
badges,
}
}

pub fn dependencies(&self) -> CargoResult<BTreeMap<String, TomlDependency>> {
self.dependencies.clone().map_or(
Err(anyhow!("`workspace.dependencies` was not defined")),
|d| Ok(d),
)
}

pub fn get_dependency(&self, name: &str) -> CargoResult<TomlDependency> {
self.dependencies.clone().map_or(
Err(anyhow!("`workspace.dependencies` was not defined")),
|deps| {
deps.get(name).map_or(
Err(anyhow!(
"`dependency.{}` was not found in `workspace.dependencies`",
name
)),
|dep| Ok(dep.clone()),
)
},
)
}

pub fn version(&self) -> CargoResult<semver::Version> {
self.version
.clone()
.map_or(Err(anyhow!("`workspace.version` was not defined")), |d| {
Ok(d)
})
}

pub fn authors(&self) -> CargoResult<Vec<String>> {
self.authors
.clone()
.map_or(Err(anyhow!("`workspace.authors` was not defined")), |d| {
Ok(d)
})
}

pub fn description(&self) -> CargoResult<String> {
self.description.clone().map_or(
Err(anyhow!("`workspace.description` was not defined")),
|d| Ok(d),
)
}

pub fn homepage(&self) -> CargoResult<String> {
self.homepage
.clone()
.map_or(Err(anyhow!("`workspace.homepage` was not defined")), |d| {
Ok(d)
})
}

pub fn documentation(&self) -> CargoResult<String> {
self.documentation.clone().map_or(
Err(anyhow!("`workspace.documentation` was not defined")),
|d| Ok(d),
)
}

pub fn readme(&self) -> CargoResult<StringOrBool> {
self.readme
.clone()
.map_or(Err(anyhow!("`workspace.readme` was not defined")), |d| {
Ok(d)
})
}

pub fn keywords(&self) -> CargoResult<Vec<String>> {
self.keywords
.clone()
.map_or(Err(anyhow!("`workspace.keywords` was not defined")), |d| {
Ok(d)
})
}

pub fn categories(&self) -> CargoResult<Vec<String>> {
self.categories.clone().map_or(
Err(anyhow!("`workspace.categories` was not defined")),
|d| Ok(d),
)
}

pub fn license(&self) -> CargoResult<String> {
self.license
.clone()
.map_or(Err(anyhow!("`workspace.license` was not defined")), |d| {
Ok(d)
})
}

pub fn license_file(&self) -> CargoResult<String> {
self.license_file.clone().map_or(
Err(anyhow!("`workspace.license_file` was not defined")),
|d| Ok(d),
)
}

pub fn repository(&self) -> CargoResult<String> {
self.repository.clone().map_or(
Err(anyhow!("`workspace.repository` was not defined")),
|d| Ok(d),
)
}

pub fn publish(&self) -> CargoResult<VecStringOrBool> {
self.publish
.clone()
.map_or(Err(anyhow!("`workspace.publish` was not defined")), |d| {
Ok(d)
})
}

pub fn edition(&self) -> CargoResult<String> {
self.edition
.clone()
.map_or(Err(anyhow!("`workspace.edition` was not defined")), |d| {
Ok(d)
})
}

pub fn badges(&self) -> CargoResult<BTreeMap<String, BTreeMap<String, String>>> {
self.badges
.clone()
.map_or(Err(anyhow!("`workspace.badges` was not defined")), |d| {
Ok(d)
})
}
}
Loading

0 comments on commit b1636fc

Please sign in to comment.