Implement package description checking

Author: infinisil

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
 

nixpkgs-check.nix

@@ -49,7 +49,7 @@ let
         ${initNix}
         # This is what nixpkgs-vet uses
         export NIXPKGS_VET_NIX_PACKAGE=${lib.getBin nix}
-        ${nixpkgs-vet}/bin/.nixpkgs-vet-wrapped --base "${nixpkgs}" "${nixpkgs}"
+        time ${nixpkgs-vet}/bin/.nixpkgs-vet-wrapped --base "${nixpkgs}" "${nixpkgs}"
         touch $out
       '';
 in

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; }

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<String>,
+    },
+}
+
 #[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<String>) -> 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,
     }))
 }

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}")
+            },
        }
     }
 }