diff --git a/README.md b/README.md index 4613849..645f4a8 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,11 @@ The following checks are performed when calling the binary: Evaluate Nixpkgs with `system` set to `x86_64-linux` and check that: - For each package directory, the `pkgs.${name}` attribute must be defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`. - For each package directory, `pkgs.lib.isDerivation pkgs.${name}` must be `true`. +- For each top-level attribute, `meta.description` must: + - Start with a capital letter + - Not start with an article (a/an/the) + - Not start with the package name + - Not end with punctuation ### Ratchet checks diff --git a/src/eval.nix b/src/eval.nix index f3c6f5d..b574f53 100644 --- a/src/eval.nix +++ b/src/eval.nix @@ -59,7 +59,18 @@ let else { AttributeSet = { - is_derivation = pkgs.lib.isDerivation value; + derivation = + if pkgs.lib.isDerivation value then + { + Derivation = { + pname = pkgs.lib.getName value; + description = value.meta.description or null; + }; + } + else + { + NonDerivation = null; + }; definition_variant = if !value ? _callPackageVariant then { ManualDefinition.is_semantic_call_package = false; } diff --git a/src/eval.rs b/src/eval.rs index db9152e..4bd9154 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -79,12 +79,22 @@ pub enum AttributeVariant { NonAttributeSet, AttributeSet { /// Whether the attribute is a derivation (`lib.isDerivation`) - is_derivation: bool, + derivation: Derivation, /// The type of `callPackage` used. definition_variant: DefinitionVariant, }, } +/// Info about a derivation +#[derive(Deserialize)] +pub enum Derivation { + NonDerivation, + Derivation { + pname: String, + description: Option, + }, +} + #[derive(Deserialize)] pub enum DefinitionVariant { /// An automatic definition by the `pkgs/by-name` overlay, though it's detected using the @@ -261,6 +271,61 @@ pub fn check_values( })) } +/// Check the validity of a package description +fn check_description(pname: &str, description: &Option) -> ratchet::PackageDescription { + if let Some(text) = description { + let lowercased = text.to_lowercase(); + ratchet::PackageDescription { + not_capitalised: { + if let Some(first) = text.chars().next() { + if first.is_lowercase() { + Loose(text.to_owned()) + } else { + Tight + } + } else { + Tight + } + }, + starts_with_article: { + if lowercased.starts_with("a ") + || lowercased.starts_with("an ") + || lowercased.starts_with("the ") + { + Loose(text.to_owned()) + } else { + Tight + } + }, + starts_with_package_name: { + if lowercased.starts_with(&pname.to_lowercase()) { + Loose(text.to_owned()) + } else { + Tight + } + }, + ends_with_punctuation: { + if let Some(last) = text.chars().last() { + if last.is_ascii_punctuation() { + Loose(text.to_owned()) + } else { + Tight + } + } else { + Tight + } + }, + } + } else { + ratchet::PackageDescription { + not_capitalised: Tight, + starts_with_article: Tight, + starts_with_package_name: Tight, + ends_with_punctuation: Tight, + } + } +} + /// Handle the evaluation result for an attribute in `pkgs/by-name`, making it a validation result. fn by_name( nix_file_store: &mut NixFileStore, @@ -279,6 +344,27 @@ fn by_name( .into() }; + let description_ratchet = match by_name_attribute { + Existing(AttributeInfo { + attribute_variant: + AttributeVariant::AttributeSet { + derivation: + Derivation::Derivation { + pname: ref name, + ref description, + }, + .. + }, + .. + }) => check_description(name, description), + _ => ratchet::PackageDescription { + not_capitalised: NonApplicable, + starts_with_article: NonApplicable, + starts_with_package_name: NonApplicable, + ends_with_punctuation: NonApplicable, + }, + }; + // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists. This match // decides whether the attribute `foo` is defined accordingly and whether a legacy manual // definition could be removed. @@ -308,16 +394,17 @@ fn by_name( // And it's an attribute set, which allows us to get more information about it attribute_variant: AttributeVariant::AttributeSet { - is_derivation, + derivation, definition_variant, }, location, }) => { // Only derivations are allowed in `pkgs/by-name`. - let is_derivation_result = if is_derivation { - Success(()) - } else { - to_validation(ByNameErrorKind::NonDerivation).map(|_| ()) + let is_derivation_result = match derivation { + Derivation::Derivation { .. } => Success(()), + Derivation::NonDerivation => { + to_validation(ByNameErrorKind::NonDerivation).map(|_| ()) + } }; // If the definition looks correct @@ -399,6 +486,7 @@ fn by_name( // once at the end with a map. manual_definition_result.map(|manual_definition| ratchet::Package { manual_definition, + description: description_ratchet, uses_by_name: Tight, }), ) @@ -477,6 +565,27 @@ fn handle_non_by_name_attribute( use ratchet::RatchetState::*; use NonByNameAttribute::*; + let description_ratchet = match non_by_name_attribute { + EvalSuccess(AttributeInfo { + attribute_variant: + AttributeVariant::AttributeSet { + derivation: + Derivation::Derivation { + pname: ref name, + ref description, + }, + .. + }, + .. + }) => check_description(name, description), + _ => ratchet::PackageDescription { + not_capitalised: NonApplicable, + starts_with_article: NonApplicable, + starts_with_package_name: NonApplicable, + ends_with_punctuation: NonApplicable, + }, + }; + // The ratchet state whether this attribute uses `pkgs/by-name`. // // This is never `Tight`, because we only either: @@ -504,7 +613,7 @@ fn handle_non_by_name_attribute( // are. Anything else can't be in `pkgs/by-name`. attribute_variant: AttributeVariant::AttributeSet { // As of today, non-derivation attribute sets can't be in `pkgs/by-name`. - is_derivation: true, + derivation: Derivation::Derivation { .. }, // Of the two definition variants, really only the manual one makes sense here. // // Special cases are: @@ -601,5 +710,6 @@ fn handle_non_by_name_attribute( // ourselves all the time to define `manual_definition`, just set it once at the end here. manual_definition: Tight, uses_by_name, + description: description_ratchet, })) } diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index f5046de..7e4f950 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -19,6 +19,7 @@ pub enum NixpkgsProblem { NixFile(NixFileError), TopLevelPackage(TopLevelPackageError), NixEval(NixEvalError), + Description(DescriptionError), } /// A file structure error involving a shard (e.g. `fo` is the shard in the path `pkgs/by-name/fo/foo/package.nix`) @@ -146,6 +147,23 @@ pub struct NixEvalError { pub stderr: String, } +/// An error relating to the description of a package +#[derive(Clone)] +pub struct DescriptionError { + pub package_name: String, + pub description: String, + pub kind: DescriptionErrorKind, +} + +/// The kind of description problem +#[derive(Clone)] +pub enum DescriptionErrorKind { + NotCapitalised, + StartsWithArticle, + StartsWithPackageName, + EndsWithPunctuation, +} + impl fmt::Display for NixpkgsProblem { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { @@ -440,6 +458,15 @@ impl fmt::Display for NixpkgsProblem { f.write_str(stderr)?; write!(f, "- Nix evaluation failed for some package in `pkgs/by-name`, see error above") }, + NixpkgsProblem::Description(DescriptionError { package_name, description, kind }) => { + let text = match kind { + DescriptionErrorKind::NotCapitalised => "is not capitalised", + DescriptionErrorKind::StartsWithArticle => "starts with an article (the/a/an)", + DescriptionErrorKind::StartsWithPackageName => "starts with the package name", + DescriptionErrorKind::EndsWithPunctuation => "ends with punctuation", + }; + write!(f, "- Description for package {package_name} (\"{description}\") {text}") + }, } } } diff --git a/src/ratchet.rs b/src/ratchet.rs index 4e6ce04..d683ba3 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -3,7 +3,8 @@ //! Each type has a `compare` method that validates the ratchet checks for that item. use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::{NixpkgsProblem, TopLevelPackageError}; +use crate::nixpkgs_problem::DescriptionErrorKind; +use crate::nixpkgs_problem::{DescriptionError, NixpkgsProblem, TopLevelPackageError}; use crate::validation::{self, Validation, Validation::Success}; use relative_path::RelativePathBuf; use std::collections::HashMap; @@ -37,6 +38,9 @@ pub struct Package { /// The ratchet value for the check for new packages using pkgs/by-name pub uses_by_name: RatchetState, + + /// The package description is valid + pub description: PackageDescription, } impl Package { @@ -53,6 +57,11 @@ impl Package { optional_from.map(|x| &x.uses_by_name), &to.uses_by_name, ), + PackageDescription::compare( + name, + optional_from.map(|x| &x.description), + &to.description, + ), ]) } } @@ -166,3 +175,92 @@ impl ToNixpkgsProblem for UsesByName { }) } } + +/// The ratchet value of an attribute for the check that the description is valid for new packages. +/// +/// This checks that new packages don't use an indefinite article in `meta.description`. +pub struct PackageDescription { + pub not_capitalised: RatchetState, + pub starts_with_article: RatchetState, + pub starts_with_package_name: RatchetState, + pub ends_with_punctuation: RatchetState, +} + +impl PackageDescription { + pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { + validation::sequence_([ + RatchetState::::compare( + name, + optional_from.map(|x| &x.not_capitalised), + &to.not_capitalised, + ), + RatchetState::::compare( + name, + optional_from.map(|x| &x.starts_with_article), + &to.starts_with_article, + ), + RatchetState::::compare( + name, + optional_from.map(|x| &x.starts_with_package_name), + &to.starts_with_package_name, + ), + RatchetState::::compare( + name, + optional_from.map(|x| &x.ends_with_punctuation), + &to.ends_with_punctuation, + ), + ]) + } +} + +pub enum DescriptionNotCapitalised {} +pub enum DescriptionStartsWithArticle {} +pub enum DescriptionStartsWithPackageName {} +pub enum DescriptionEndsWithPunctuation {} + +impl ToDescriptionErrorKind for DescriptionNotCapitalised { + fn to_description_error_kind() -> DescriptionErrorKind { + DescriptionErrorKind::NotCapitalised + } +} + +impl ToDescriptionErrorKind for DescriptionStartsWithArticle { + fn to_description_error_kind() -> DescriptionErrorKind { + DescriptionErrorKind::StartsWithArticle + } +} + +impl ToDescriptionErrorKind for DescriptionStartsWithPackageName { + fn to_description_error_kind() -> DescriptionErrorKind { + DescriptionErrorKind::StartsWithPackageName + } +} + +impl ToDescriptionErrorKind for DescriptionEndsWithPunctuation { + fn to_description_error_kind() -> DescriptionErrorKind { + DescriptionErrorKind::EndsWithPunctuation + } +} + +pub trait ToDescriptionErrorKind { + fn to_description_error_kind() -> DescriptionErrorKind; +} + +impl ToNixpkgsProblem for T +where + T: ToDescriptionErrorKind, +{ + type ToContext = String; + + fn to_nixpkgs_problem( + name: &str, + _optional_from: Option<()>, + description: &Self::ToContext, + ) -> NixpkgsProblem { + NixpkgsProblem::Description(DescriptionError { + package_name: name.to_owned(), + description: description.to_owned(), + kind: T::to_description_error_kind(), + }) + } +} diff --git a/tests/description-already-nonconform/base/default.nix b/tests/description-already-nonconform/base/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-already-nonconform/base/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-already-nonconform/base/expected b/tests/description-already-nonconform/base/expected new file mode 100644 index 0000000..defae26 --- /dev/null +++ b/tests/description-already-nonconform/base/expected @@ -0,0 +1 @@ +Validated successfully diff --git a/tests/description-already-nonconform/base/pkgs/by-name/fo/foo/package.nix b/tests/description-already-nonconform/base/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..123fbe2 --- /dev/null +++ b/tests/description-already-nonconform/base/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo"; + meta.description = "a test."; +} diff --git a/tests/description-already-nonconform/base/pkgs/by-name/fo/foo2/package.nix b/tests/description-already-nonconform/base/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..b7f1bc9 --- /dev/null +++ b/tests/description-already-nonconform/base/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo2"; + meta.description = "Foo2 is a test"; +} diff --git a/tests/description-already-nonconform/default.nix b/tests/description-already-nonconform/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-already-nonconform/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-already-nonconform/expected b/tests/description-already-nonconform/expected new file mode 100644 index 0000000..defae26 --- /dev/null +++ b/tests/description-already-nonconform/expected @@ -0,0 +1 @@ +Validated successfully diff --git a/tests/description-already-nonconform/pkgs/by-name/fo/foo/package.nix b/tests/description-already-nonconform/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..123fbe2 --- /dev/null +++ b/tests/description-already-nonconform/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo"; + meta.description = "a test."; +} diff --git a/tests/description-already-nonconform/pkgs/by-name/fo/foo2/package.nix b/tests/description-already-nonconform/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..b7f1bc9 --- /dev/null +++ b/tests/description-already-nonconform/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo2"; + meta.description = "Foo2 is a test"; +} diff --git a/tests/description-new-package/default.nix b/tests/description-new-package/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-new-package/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-new-package/expected b/tests/description-new-package/expected new file mode 100644 index 0000000..b5bb1b0 --- /dev/null +++ b/tests/description-new-package/expected @@ -0,0 +1,5 @@ +- Description for package foo ("a test.") is not capitalised +- Description for package foo ("a test.") starts with an article (the/a/an) +- Description for package foo ("a test.") ends with punctuation +- Description for package foo2 ("Foo2 is a test") starts with the package name +This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch. diff --git a/tests/description-new-package/pkgs/by-name/fo/foo/package.nix b/tests/description-new-package/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..123fbe2 --- /dev/null +++ b/tests/description-new-package/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo"; + meta.description = "a test."; +} diff --git a/tests/description-new-package/pkgs/by-name/fo/foo2/package.nix b/tests/description-new-package/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..b7f1bc9 --- /dev/null +++ b/tests/description-new-package/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo2"; + meta.description = "Foo2 is a test"; +} diff --git a/tests/description-newly-nonconform/base/default.nix b/tests/description-newly-nonconform/base/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-newly-nonconform/base/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-newly-nonconform/base/expected b/tests/description-newly-nonconform/base/expected new file mode 100644 index 0000000..defae26 --- /dev/null +++ b/tests/description-newly-nonconform/base/expected @@ -0,0 +1 @@ +Validated successfully diff --git a/tests/description-newly-nonconform/base/pkgs/by-name/fo/foo/package.nix b/tests/description-newly-nonconform/base/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..cc855ed --- /dev/null +++ b/tests/description-newly-nonconform/base/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo"; + meta.description = "Test"; +} diff --git a/tests/description-newly-nonconform/base/pkgs/by-name/fo/foo2/package.nix b/tests/description-newly-nonconform/base/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..c061e38 --- /dev/null +++ b/tests/description-newly-nonconform/base/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo2"; + meta.description = "Test 2"; +} diff --git a/tests/description-newly-nonconform/default.nix b/tests/description-newly-nonconform/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-newly-nonconform/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-newly-nonconform/expected b/tests/description-newly-nonconform/expected new file mode 100644 index 0000000..b5bb1b0 --- /dev/null +++ b/tests/description-newly-nonconform/expected @@ -0,0 +1,5 @@ +- Description for package foo ("a test.") is not capitalised +- Description for package foo ("a test.") starts with an article (the/a/an) +- Description for package foo ("a test.") ends with punctuation +- Description for package foo2 ("Foo2 is a test") starts with the package name +This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch. diff --git a/tests/description-newly-nonconform/pkgs/by-name/fo/foo/package.nix b/tests/description-newly-nonconform/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..123fbe2 --- /dev/null +++ b/tests/description-newly-nonconform/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo"; + meta.description = "a test."; +} diff --git a/tests/description-newly-nonconform/pkgs/by-name/fo/foo2/package.nix b/tests/description-newly-nonconform/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..b7f1bc9 --- /dev/null +++ b/tests/description-newly-nonconform/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo2"; + meta.description = "Foo2 is a test"; +} diff --git a/tests/description-newly-set/base/default.nix b/tests/description-newly-set/base/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-newly-set/base/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-newly-set/base/expected b/tests/description-newly-set/base/expected new file mode 100644 index 0000000..defae26 --- /dev/null +++ b/tests/description-newly-set/base/expected @@ -0,0 +1 @@ +Validated successfully diff --git a/tests/description-newly-set/base/pkgs/by-name/fo/foo/package.nix b/tests/description-newly-set/base/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..a1b92ef --- /dev/null +++ b/tests/description-newly-set/base/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/tests/description-newly-set/base/pkgs/by-name/fo/foo2/package.nix b/tests/description-newly-set/base/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..a1b92ef --- /dev/null +++ b/tests/description-newly-set/base/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/tests/description-newly-set/default.nix b/tests/description-newly-set/default.nix new file mode 100644 index 0000000..861260c --- /dev/null +++ b/tests/description-newly-set/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/tests/description-newly-set/expected b/tests/description-newly-set/expected new file mode 100644 index 0000000..b5bb1b0 --- /dev/null +++ b/tests/description-newly-set/expected @@ -0,0 +1,5 @@ +- Description for package foo ("a test.") is not capitalised +- Description for package foo ("a test.") starts with an article (the/a/an) +- Description for package foo ("a test.") ends with punctuation +- Description for package foo2 ("Foo2 is a test") starts with the package name +This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch. diff --git a/tests/description-newly-set/pkgs/by-name/fo/foo/package.nix b/tests/description-newly-set/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000..123fbe2 --- /dev/null +++ b/tests/description-newly-set/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo"; + meta.description = "a test."; +} diff --git a/tests/description-newly-set/pkgs/by-name/fo/foo2/package.nix b/tests/description-newly-set/pkgs/by-name/fo/foo2/package.nix new file mode 100644 index 0000000..b7f1bc9 --- /dev/null +++ b/tests/description-newly-set/pkgs/by-name/fo/foo2/package.nix @@ -0,0 +1,4 @@ +{ someDrv }: someDrv // { + pname = "foo2"; + meta.description = "Foo2 is a test"; +} diff --git a/tests/mock-nixpkgs.nix b/tests/mock-nixpkgs.nix index d67846d..faac486 100644 --- a/tests/mock-nixpkgs.nix +++ b/tests/mock-nixpkgs.nix @@ -42,6 +42,7 @@ let # which then doesn't trigger this check // lib.mapAttrs (name: value: value) { someDrv = { + pname = "someDrv"; type = "derivation"; }; };