Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(toml): Flatten manifest parsing #13589

Merged
merged 19 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ cargo-platform = { path = "crates/cargo-platform", version = "0.1.5" }
cargo-test-macro = { path = "crates/cargo-test-macro" }
cargo-test-support = { path = "crates/cargo-test-support" }
cargo-util = { version = "0.2.9", path = "crates/cargo-util" }
cargo-util-schemas = { version = "0.2.1", path = "crates/cargo-util-schemas" }
cargo-util-schemas = { version = "0.3.0", path = "crates/cargo-util-schemas" }
cargo_metadata = "0.18.1"
clap = "4.5.1"
color-print = "0.3.5"
Expand Down Expand Up @@ -97,7 +97,7 @@ tempfile = "3.10.1"
thiserror = "1.0.57"
time = { version = "0.3", features = ["parsing", "formatting", "serde"] }
toml = "0.8.10"
toml_edit = { version = "0.22.6", features = ["serde"] }
toml_edit = { version = "0.22.7", features = ["serde"] }
tracing = "0.1.40" # be compatible with rustc_log: https://github.com/rust-lang/rust/blob/e51e98dde6a/compiler/rustc_log/Cargo.toml#L9
tracing-chrome = "0.7.1"
tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
Expand Down
2 changes: 1 addition & 1 deletion crates/cargo-util-schemas/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "cargo-util-schemas"
version = "0.2.1"
version = "0.3.0"
rust-version = "1.76.0" # MSRV:1
edition.workspace = true
license.workspace = true
Expand Down
8 changes: 7 additions & 1 deletion crates/cargo-util-schemas/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//! - Keys that exist for bookkeeping but don't correspond to the schema have a `_` prefix

use std::collections::BTreeMap;
use std::collections::BTreeSet;
Muscraft marked this conversation as resolved.
Show resolved Hide resolved
use std::fmt::{self, Display, Write};
use std::path::PathBuf;
use std::str;
Expand All @@ -28,6 +29,7 @@ pub use rust_version::RustVersionError;
#[derive(Debug, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub struct TomlManifest {
// when adding new fields, be sure to check whether `requires_package` should disallow them
pub cargo_features: Option<Vec<String>>,
pub package: Option<Box<TomlPackage>>,
pub project: Option<Box<TomlPackage>>,
Expand All @@ -51,7 +53,11 @@ pub struct TomlManifest {
pub workspace: Option<TomlWorkspace>,
pub badges: Option<InheritableBtreeMap>,
pub lints: Option<InheritableLints>,
// when adding new fields, be sure to check whether `requires_package` should disallow them

/// Report unused keys (see also nested `_unused_keys`)
/// Note: this is populated by the caller, rather than automatically
#[serde(skip)]
pub _unused_keys: BTreeSet<String>,
}

impl TomlManifest {
Expand Down
4 changes: 2 additions & 2 deletions crates/xtask-stale-label/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
use std::fmt::Write as _;
use std::path::PathBuf;
use std::process;
use toml_edit::Document;
use toml_edit::DocumentMut;

fn main() {
let pkg_root = std::env!("CARGO_MANIFEST_DIR");
Expand All @@ -31,7 +31,7 @@ fn main() {
let mut passed = 0;

let toml = std::fs::read_to_string(path).expect("read from file");
let doc = toml.parse::<Document>().expect("a toml");
let doc = toml.parse::<DocumentMut>().expect("a toml");
let autolabel = doc["autolabel"].as_table().expect("a toml table");

for (label, value) in autolabel.iter() {
Expand Down
4 changes: 2 additions & 2 deletions src/bin/cargo/commands/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ fn parse_section(args: &ArgMatches) -> DepTable {
/// Clean up the workspace.dependencies, profile, patch, and replace sections of the root manifest
/// by removing dependencies which no longer have a reference to them.
fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {
let mut manifest: toml_edit::Document =
let mut manifest: toml_edit::DocumentMut =
cargo_util::paths::read(workspace.root_manifest())?.parse()?;
let mut is_modified = true;

Expand Down Expand Up @@ -315,7 +315,7 @@ fn spec_has_match(

/// Removes unused patches from the manifest
fn gc_unused_patches(workspace: &Workspace<'_>, resolve: &Resolve) -> CargoResult<bool> {
let mut manifest: toml_edit::Document =
let mut manifest: toml_edit::DocumentMut =
cargo_util::paths::read(workspace.root_manifest())?.parse()?;
let mut modified = false;

Expand Down
15 changes: 10 additions & 5 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ impl EitherManifest {
/// This is deserialized using the [`TomlManifest`] type.
#[derive(Clone, Debug)]
pub struct Manifest {
// alternate forms of manifests:
resolved_toml: Rc<TomlManifest>,
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
summary: Summary,

// this form of manifest:
targets: Vec<Target>,
default_kind: Option<CompileKind>,
forced_kind: Option<CompileKind>,
Expand All @@ -56,7 +60,6 @@ pub struct Manifest {
replace: Vec<(PackageIdSpec, Dependency)>,
patch: HashMap<Url, Vec<Dependency>>,
workspace: WorkspaceConfig,
original: Rc<TomlManifest>,
unstable_features: Features,
edition: Edition,
rust_version: Option<RustVersion>,
Expand Down Expand Up @@ -386,7 +389,9 @@ compact_debug! {

impl Manifest {
pub fn new(
resolved_toml: Rc<TomlManifest>,
summary: Summary,

default_kind: Option<CompileKind>,
forced_kind: Option<CompileKind>,
targets: Vec<Target>,
Expand All @@ -405,14 +410,15 @@ impl Manifest {
rust_version: Option<RustVersion>,
im_a_teapot: Option<bool>,
default_run: Option<String>,
original: Rc<TomlManifest>,
metabuild: Option<Vec<String>>,
resolve_behavior: Option<ResolveBehavior>,
lint_rustflags: Vec<String>,
embedded: bool,
) -> Manifest {
Manifest {
resolved_toml,
summary,

default_kind,
forced_kind,
targets,
Expand All @@ -430,7 +436,6 @@ impl Manifest {
unstable_features,
edition,
rust_version,
original,
im_a_teapot,
default_run,
metabuild,
Expand Down Expand Up @@ -495,8 +500,8 @@ impl Manifest {
pub fn replace(&self) -> &[(PackageIdSpec, Dependency)] {
&self.replace
}
pub fn original(&self) -> &TomlManifest {
&self.original
pub fn resolved_toml(&self) -> &TomlManifest {
&self.resolved_toml
}
pub fn patch(&self) -> &HashMap<Url, Vec<Dependency>> {
&self.patch
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ impl Package {
}

pub fn to_registry_toml(&self, ws: &Workspace<'_>) -> CargoResult<String> {
let manifest = prepare_for_publish(self.manifest().original(), ws, self.root())?;
let manifest = prepare_for_publish(self.manifest().resolved_toml(), ws, self.root())?;
let toml = toml::to_string_pretty(&manifest)?;
Ok(format!("{}\n{}", MANIFEST_PREAMBLE, toml))
}
Expand Down
10 changes: 4 additions & 6 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,6 @@ impl<'gctx> Workspace<'gctx> {
let source = SourceId::for_path(self.root())?;

let mut warnings = Vec::new();
let mut nested_paths = Vec::new();

let mut patch = HashMap::new();
for (url, deps) in config_patch.into_iter().flatten() {
Expand All @@ -442,7 +441,6 @@ impl<'gctx> Workspace<'gctx> {
dep,
name,
source,
&mut nested_paths,
self.gctx,
&mut warnings,
/* platform */ None,
Expand Down Expand Up @@ -1006,7 +1004,7 @@ impl<'gctx> Workspace<'gctx> {
);
self.gctx.shell().warn(&msg)
};
if manifest.original().has_profiles() {
if manifest.resolved_toml().has_profiles() {
emit_warning("profiles")?;
}
if !manifest.replace().is_empty() {
Expand Down Expand Up @@ -1063,7 +1061,7 @@ impl<'gctx> Workspace<'gctx> {
return Ok(p);
}
let source_id = SourceId::for_path(manifest_path.parent().unwrap())?;
let (package, _nested_paths) = ops::read_package(manifest_path, source_id, self.gctx)?;
let package = ops::read_package(manifest_path, source_id, self.gctx)?;
loaded.insert(manifest_path.to_path_buf(), package.clone());
Ok(package)
}
Expand Down Expand Up @@ -1596,7 +1594,7 @@ impl<'gctx> Packages<'gctx> {
Entry::Occupied(e) => Ok(e.into_mut()),
Entry::Vacant(v) => {
let source_id = SourceId::for_path(key)?;
let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, self.gctx)?;
let manifest = read_manifest(manifest_path, source_id, self.gctx)?;
Ok(v.insert(match manifest {
EitherManifest::Real(manifest) => {
MaybePackage::Package(Package::new(manifest, manifest_path))
Expand Down Expand Up @@ -1746,7 +1744,7 @@ pub fn find_workspace_root(
find_workspace_root_with_loader(manifest_path, gctx, |self_path| {
let key = self_path.parent().unwrap();
let source_id = SourceId::for_path(key)?;
let (manifest, _nested_paths) = read_manifest(self_path, source_id, gctx)?;
let manifest = read_manifest(self_path, source_id, gctx)?;
Ok(manifest
.workspace_config()
.get_ws_root(self_path, manifest_path))
Expand Down
12 changes: 6 additions & 6 deletions src/cargo/ops/cargo_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,7 @@ fn mk(gctx: &GlobalContext, opts: &MkOptions<'_>) -> CargoResult<()> {
write_ignore_file(path, &ignore, vcs)?;

// Create `Cargo.toml` file with necessary `[lib]` and `[[bin]]` sections, if needed.
let mut manifest = toml_edit::Document::new();
let mut manifest = toml_edit::DocumentMut::new();
manifest["package"] = toml_edit::Item::Table(toml_edit::Table::new());
manifest["package"]["name"] = toml_edit::value(name);
manifest["package"]["version"] = toml_edit::value("0.1.0");
Expand Down Expand Up @@ -814,7 +814,7 @@ fn mk(gctx: &GlobalContext, opts: &MkOptions<'_>) -> CargoResult<()> {
// Sometimes the root manifest is not a valid manifest, so we only try to parse it if it is.
// This should not block the creation of the new project. It is only a best effort to
// inherit the workspace package keys.
if let Ok(mut workspace_document) = root_manifest.parse::<toml_edit::Document>() {
if let Ok(mut workspace_document) = root_manifest.parse::<toml_edit::DocumentMut>() {
let display_path = get_display_path(&root_manifest_path, &path)?;
let can_be_a_member = can_be_workspace_member(&display_path, &workspace_document)?;
// Only try to inherit the workspace stuff if the new package can be a member of the workspace.
Expand Down Expand Up @@ -933,14 +933,14 @@ mod tests {
// If the option is set, keep the value from the manifest.
fn update_manifest_with_inherited_workspace_package_keys(
opts: &MkOptions<'_>,
manifest: &mut toml_edit::Document,
manifest: &mut toml_edit::DocumentMut,
workspace_package_keys: &toml_edit::Table,
) {
if workspace_package_keys.is_empty() {
return;
}

let try_remove_and_inherit_package_key = |key: &str, manifest: &mut toml_edit::Document| {
let try_remove_and_inherit_package_key = |key: &str, manifest: &mut toml_edit::DocumentMut| {
let package = manifest["package"]
.as_table_mut()
.expect("package is a table");
Expand Down Expand Up @@ -974,7 +974,7 @@ fn update_manifest_with_inherited_workspace_package_keys(
/// with the new package in it.
fn update_manifest_with_new_member(
root_manifest_path: &Path,
workspace_document: &mut toml_edit::Document,
workspace_document: &mut toml_edit::DocumentMut,
display_path: &str,
) -> CargoResult<bool> {
// If the members element already exist, check if one of the patterns
Expand Down Expand Up @@ -1048,7 +1048,7 @@ fn get_display_path(root_manifest_path: &Path, package_path: &Path) -> CargoResu
// Check if the package can be a member of the workspace.
fn can_be_workspace_member(
display_path: &str,
workspace_document: &toml_edit::Document,
workspace_document: &toml_edit::DocumentMut,
) -> CargoResult<bool> {
if let Some(exclude) = workspace_document
.get("workspace")
Expand Down
7 changes: 3 additions & 4 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,11 +453,10 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
let orig_resolve = ops::load_pkg_lockfile(ws)?;

// Convert Package -> TomlManifest -> Manifest -> Package
let toml_manifest = prepare_for_publish(orig_pkg.manifest().original(), ws, orig_pkg.root())?;
let package_root = orig_pkg.root();
let toml_manifest =
prepare_for_publish(orig_pkg.manifest().resolved_toml(), ws, orig_pkg.root())?;
let source_id = orig_pkg.package_id().source_id();
let (manifest, _nested_paths) =
to_real_manifest(toml_manifest, false, source_id, package_root, gctx)?;
let manifest = to_real_manifest(toml_manifest, source_id, orig_pkg.manifest_path(), gctx)?;
let new_pkg = Package::new(manifest, orig_pkg.manifest_path());

// Regenerate Cargo.lock using the old one as a guide.
Expand Down
Loading