From 0fded466fc51d21b91931e4efffb481c0960014a Mon Sep 17 00:00:00 2001 From: Rain Date: Sat, 20 Nov 2021 10:23:29 -0800 Subject: [PATCH] [guppy-summaries] make metadata a dynamic toml value This means that the same library can parse summaries created by both old and new versions of guppy. We don't use the summary for much anyway, currently. I also looked at switching to `toml_edit`, which has a much better design overall. Unfortunately, I ran into https://github.com/ordian/toml_edit/issues/192. It makes sense to switch to `toml_edit` at some point in the future, and I've filed #492 to keep track of that. --- guppy-summaries/src/diff.rs | 6 +-- guppy-summaries/src/lib.rs | 5 +- guppy-summaries/src/summary.rs | 51 ++++++++----------- guppy-summaries/src/unit_tests/basic_tests.rs | 8 ++- guppy/src/errors.rs | 7 +++ guppy/src/graph/summaries.rs | 22 ++++---- 6 files changed, 45 insertions(+), 54 deletions(-) diff --git a/guppy-summaries/src/diff.rs b/guppy-summaries/src/diff.rs index 929c7f2bd0a..e80e82c9bc4 100644 --- a/guppy-summaries/src/diff.rs +++ b/guppy-summaries/src/diff.rs @@ -9,9 +9,7 @@ //! summaries or through `SummaryDiff::new`. pub use crate::report::SummaryReport; -use crate::{ - PackageInfo, PackageMap, PackageStatus, SummaryId, SummarySource, SummaryWithMetadata, -}; +use crate::{PackageInfo, PackageMap, PackageStatus, Summary, SummaryId, SummarySource}; use diffus::{edit, Diffable}; use semver::Version; use serde::{ser::SerializeStruct, Serialize}; @@ -82,7 +80,7 @@ pub struct SummaryDiff<'a> { impl<'a> SummaryDiff<'a> { /// Computes a diff between two summaries. - pub fn new(old: &'a SummaryWithMetadata, new: &'a SummaryWithMetadata) -> Self { + pub fn new(old: &'a Summary, new: &'a Summary) -> Self { Self { target_packages: PackageDiff::new(&old.target_packages, &new.target_packages), host_packages: PackageDiff::new(&old.host_packages, &new.host_packages), diff --git a/guppy-summaries/src/lib.rs b/guppy-summaries/src/lib.rs index e085e1c9a5e..fb526c6b3b1 100644 --- a/guppy-summaries/src/lib.rs +++ b/guppy-summaries/src/lib.rs @@ -12,15 +12,12 @@ //! # Examples //! //! ```rust -//! use guppy_summaries::{SummaryWithMetadata, SummaryId, SummarySource, PackageStatus}; +//! use guppy_summaries::{Summary, SummaryId, SummarySource, PackageStatus}; //! use pretty_assertions::assert_eq; //! use semver::Version; //! use std::collections::BTreeSet; //! use toml::Value; //! -//! // Type alias for use in this example. -//! type Summary = SummaryWithMetadata; -//! //! // A summary is a TOML file that has this format: //! static SUMMARY: &str = r#" //! [[target-package]] diff --git a/guppy-summaries/src/summary.rs b/guppy-summaries/src/summary.rs index 724711ef188..66e82d9ffc3 100644 --- a/guppy-summaries/src/summary.rs +++ b/guppy-summaries/src/summary.rs @@ -9,7 +9,7 @@ use std::{ collections::{BTreeMap, BTreeSet}, fmt, }; -use toml::{Serializer, Value}; +use toml::{value::Table, Serializer}; /// A type representing a package map as used in `Summary` instances. pub type PackageMap = BTreeMap; @@ -19,16 +19,16 @@ pub type PackageMap = BTreeMap; /// The metadata parameter is customizable. /// /// For more, see the crate-level documentation. -#[derive(Clone, Debug, Deserialize, Eq, Hash, Serialize, PartialEq)] +#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] #[serde(rename_all = "kebab-case")] -pub struct SummaryWithMetadata { +pub struct Summary { /// Extra metadata associated with the summary. /// /// This may be used for storing extra information about the summary. /// /// The type defaults to `toml::Value` but is customizable. - #[serde(default = "Option::default")] - pub metadata: Option, + #[serde(default, skip_serializing_if = "Table::is_empty")] + pub metadata: Table, /// The packages and features built on the target platform. #[serde( @@ -49,53 +49,46 @@ pub struct SummaryWithMetadata { pub host_packages: PackageMap, } -impl SummaryWithMetadata { +impl Summary { + /// Constructs a new summary with the provided metadata, and an empty `target_packages` and + /// `host_packages`. + pub fn with_metadata(metadata: &impl Serialize) -> Result { + let toml_str = toml::to_string(metadata)?; + let metadata = + toml::from_str(&toml_str).expect("toml::to_string creates a valid TOML string"); + Ok(Self { + metadata, + ..Self::default() + }) + } + /// Deserializes a summary from the given string, with optional custom metadata. - pub fn parse<'de>(s: &'de str) -> Result - where - M: Deserialize<'de>, - { + pub fn parse(s: &str) -> Result { toml::from_str(s) } /// Perform a diff of this summary against another. /// /// This doesn't diff the metadata, just the initials and packages. - pub fn diff<'a, M2>(&'a self, other: &'a SummaryWithMetadata) -> SummaryDiff<'a> { + pub fn diff<'a>(&'a self, other: &'a Summary) -> SummaryDiff<'a> { SummaryDiff::new(self, other) } /// Serializes this summary to a TOML string. - pub fn to_string(&self) -> Result - where - M: Serialize, - { + pub fn to_string(&self) -> Result { let mut dst = String::new(); self.write_to_string(&mut dst)?; Ok(dst) } /// Serializes this summary into the given TOML string, using pretty TOML syntax. - pub fn write_to_string(&self, dst: &mut String) -> Result<(), toml::ser::Error> - where - M: Serialize, - { + pub fn write_to_string(&self, dst: &mut String) -> Result<(), toml::ser::Error> { let mut serializer = Serializer::pretty(dst); serializer.pretty_array(false); self.serialize(&mut serializer) } } -impl Default for SummaryWithMetadata { - fn default() -> Self { - Self { - metadata: None, - target_packages: PackageMap::new(), - host_packages: PackageMap::new(), - } - } -} - /// A unique identifier for a package in a build summary. #[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, Serialize, PartialEq, PartialOrd)] #[serde(rename_all = "kebab-case")] diff --git a/guppy-summaries/src/unit_tests/basic_tests.rs b/guppy-summaries/src/unit_tests/basic_tests.rs index 799ecde56ff..ec02972e372 100644 --- a/guppy-summaries/src/unit_tests/basic_tests.rs +++ b/guppy-summaries/src/unit_tests/basic_tests.rs @@ -2,15 +2,13 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 use crate::{ - diff::SummaryDiffStatus, PackageInfo, PackageMap, PackageStatus, SummaryId, SummarySource, - SummaryWithMetadata, + diff::SummaryDiffStatus, PackageInfo, PackageMap, PackageStatus, Summary, SummaryId, + SummarySource, }; use pretty_assertions::assert_eq; use semver::Version; use std::collections::BTreeSet; -type Summary = SummaryWithMetadata; - static SERIALIZED_SUMMARY: &str = r#"# This is a test @generated summary. [[target-package]] @@ -168,7 +166,7 @@ fn basic_roundtrip() { ]; let summary = Summary { - metadata: None, + metadata: Default::default(), target_packages: make_summary(target_packages), host_packages: make_summary(host_packages), }; diff --git a/guppy/src/errors.rs b/guppy/src/errors.rs index f4e0b6fbc2d..6367ee247da 100644 --- a/guppy/src/errors.rs +++ b/guppy/src/errors.rs @@ -68,6 +68,9 @@ pub enum Error { /// The registry name that wasn't recognized. registry_name: String, }, + /// An error occurred while serializing to TOML. + #[cfg(feature = "summaries")] + TomlSerializeError(toml::ser::Error), } impl Error { @@ -140,6 +143,8 @@ impl fmt::Display for Error { message, summary, registry_name ) } + #[cfg(feature = "summaries")] + TomlSerializeError(_) => write!(f, "failed to serialize to TOML"), } } } @@ -164,6 +169,8 @@ impl error::Error for Error { UnknownPackageSetSummary { .. } => None, #[cfg(feature = "summaries")] UnknownRegistryName { .. } => None, + #[cfg(feature = "summaries")] + TomlSerializeError(err) => Some(err), } } } diff --git a/guppy/src/graph/summaries.rs b/guppy/src/graph/summaries.rs index 209041be642..c95fde6e7da 100644 --- a/guppy/src/graph/summaries.rs +++ b/guppy/src/graph/summaries.rs @@ -21,9 +21,6 @@ pub use package_set::*; use serde::{Deserialize, Serialize}; use std::collections::BTreeSet; -/// A type alias for build summaries generated by `guppy`. -pub type Summary = SummaryWithMetadata; - impl<'g> CargoSet<'g> { /// Creates a build summary with the given options. /// @@ -35,11 +32,12 @@ impl<'g> CargoSet<'g> { let target_features = self.target_features(); let host_features = self.host_features(); - Ok(Summary { - metadata: Some(metadata), - target_packages: target_features.to_package_map(initials, self.target_direct_deps()), - host_packages: host_features.to_package_map(initials, self.host_direct_deps()), - }) + let mut summary = Summary::with_metadata(&metadata).map_err(Error::TomlSerializeError)?; + summary.target_packages = + target_features.to_package_map(initials, self.target_direct_deps()); + summary.host_packages = host_features.to_package_map(initials, self.host_direct_deps()); + + Ok(summary) } } @@ -320,17 +318,17 @@ mod tests { #[test] fn parse_old_metadata() { // Ensure that previous versions of the metadata parse correctly. + // TODO: note that there have been some compatibility breaks, particularly for + // omitted-packages. Probably don't need to retain too much backwards compatibility. let metadata = "\ -[metadata] version = 'v1' include-dev = true proc-macros-on-target = false "; - let parsed = Summary::parse(metadata).expect("parsed correctly"); - let metadata = parsed.metadata.expect("metadata is present"); + let summary: CargoOptionsSummary = toml::from_str(metadata).expect("parsed correctly"); assert_eq!( - InitialsPlatform::from(metadata.initials_platform), + InitialsPlatform::from(summary.initials_platform), InitialsPlatform::Standard ); }