diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 49fa4d7..8297d80 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -15,4 +15,4 @@ jobs: uses: cachix/install-nix-action@v12 - name: Run tests - run: nix-shell --run ./run-tests.py + run: nix run -c ./run-tests.py diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..8635c04 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +ast-checks/target diff --git a/ast-checks/Cargo.lock b/ast-checks/Cargo.lock new file mode 100644 index 0000000..da339b4 --- /dev/null +++ b/ast-checks/Cargo.lock @@ -0,0 +1,222 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "autocfg" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdb031dd78e28731d87d56cc8ffef4a8f36ca26c38fe2de700543e627f8a464a" + +[[package]] +name = "cbitset" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "29b6ad25ae296159fb0da12b970b2fe179b234584d7cd294c891e2bbb284466b" +dependencies = [ + "num-traits", +] + +[[package]] +name = "codespan" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "991d34632921756cbe9e3e1736b2e1f12f16166a9c9cd91979d96d4c0a086b63" +dependencies = [ + "codespan-reporting", +] + +[[package]] +name = "codespan-reporting" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c6ce42b8998a383572e0a802d859b1f00c79b7b7474e62fff88ee5c2845d9c13" +dependencies = [ + "termcolor", + "unicode-width", +] + +[[package]] +name = "itoa" +version = "0.4.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dd25036021b0de88a0aff6b850051563c6516d0bf53f8638938edbb9de732736" + +[[package]] +name = "nixpkgs-hammer-ast-checks" +version = "0.0.0" +dependencies = [ + "codespan", + "rnix", + "rowan", + "serde", + "serde_json", +] + +[[package]] +name = "num-traits" +version = "0.2.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a64b1ec5cda2586e284722486d802acf1f7dbdc623e2bfc57e65ca1cd099290" +dependencies = [ + "autocfg", +] + +[[package]] +name = "proc-macro2" +version = "1.0.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e0704ee1a7e00d7bb417d0770ea303c1bccbabf0ef1667dae92b5967f5f8a71" +dependencies = [ + "unicode-xid", +] + +[[package]] +name = "quote" +version = "1.0.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "991431c3519a3f36861882da93630ce66b52918dcf1b8e2fd66b397fc96f28df" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "rnix" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2192039d0f383be6e0edb37718152c764509ce800cafe51f58f88b27bf0e4714" +dependencies = [ + "cbitset", + "rowan", +] + +[[package]] +name = "rowan" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc3a6fb2a35518af7cab43ec4e21ca82eb086a8b3bb1739e426dc3923d459607" +dependencies = [ + "rustc-hash", + "serde", + "smol_str", + "text_unit", +] + +[[package]] +name = "rustc-hash" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" + +[[package]] +name = "ryu" +version = "1.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71d301d4193d031abdd79ff7e3dd721168a9572ef3fe51a1517aba235bd8f86e" + +[[package]] +name = "serde" +version = "1.0.121" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6159e3c76cab06f6bc466244d43b35e77e9500cd685da87620addadc2a4c40b1" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.121" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f3fcab8778dc651bc65cfab2e4eb64996f3c912b74002fb379c94517e1f27c46" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "serde_json" +version = "1.0.61" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4fceb2595057b6891a4ee808f70054bd2d12f0e97f1cbb78689b59f676df325a" +dependencies = [ + "itoa", + "ryu", + "serde", +] + +[[package]] +name = "smol_str" +version = "0.1.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ca0f7ce3a29234210f0f4f0b56f8be2e722488b95cb522077943212da3b32eb" + +[[package]] +name = "syn" +version = "1.0.59" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07cb8b1b4ebf86a89ee88cbd201b022b94138c623644d035185c84d3f41b7e66" +dependencies = [ + "proc-macro2", + "quote", + "unicode-xid", +] + +[[package]] +name = "termcolor" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4" +dependencies = [ + "winapi-util", +] + +[[package]] +name = "text_unit" +version = "0.1.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "20431e104bfecc1a40872578dbc390e10290a0e9c35fffe3ce6f73c15a9dbfc2" +dependencies = [ + "serde", +] + +[[package]] +name = "unicode-width" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9337591893a19b88d8d87f2cec1e73fad5cdfd10e5a6f349f498ad6ea2ffb1e3" + +[[package]] +name = "unicode-xid" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f7fe0bb3479651439c9112f72b6c505038574c9fbb575ed1bf3b797fa39dd564" + +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + +[[package]] +name = "winapi-util" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "70ec6ce85bb158151cae5e5c87f95a8e97d2c0c4b001223f33a334e3ce5de178" +dependencies = [ + "winapi", +] + +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" diff --git a/ast-checks/Cargo.toml b/ast-checks/Cargo.toml new file mode 100644 index 0000000..f518f70 --- /dev/null +++ b/ast-checks/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "nixpkgs-hammer-ast-checks" +version = "0.0.0" +authors = ["Robert T. McGibbon "] +edition = "2018" + +[[bin]] +name = "missing-patch-comment" +path = "src/missing-patch-comment.rs" + +[dependencies] +codespan = "0.11" +rnix = "0.7.0" +rowan = { version = "0.6.2", features = [ "serde1" ] } +serde = { version = "1.0", features = ["derive"] } +serde_json = "1.0" diff --git a/ast-checks/src/comment_finders.rs b/ast-checks/src/comment_finders.rs new file mode 100644 index 0000000..ac5e73c --- /dev/null +++ b/ast-checks/src/comment_finders.rs @@ -0,0 +1,34 @@ +use crate::tree_utils::{next_siblings, prev_siblings, walk_kind}; +use rnix::{NodeOrToken, SyntaxElement, SyntaxKind::*, SyntaxNode}; + +pub fn find_comment_within(node: &SyntaxNode) -> Option { + // Find the first `TOKEN_COMMENT` within. + let tok = walk_kind(node, TOKEN_COMMENT).next()?; + // Iterate over sibling comments and concatenate them. + let node_iter = next_siblings(&tok); + let doc = collect_comment_text(node_iter); + Some(doc).filter(|it| !it.is_empty()) +} + +pub fn find_comment_above(node: &SyntaxNode) -> Option { + // Note: The `prev_siblings` iterator includes self, which + // is not a `TOKEN_COMMENT`, so we need to skip one. + let node_iter = prev_siblings(&NodeOrToken::Node(node.clone())).skip(1); + let doc = collect_comment_text(node_iter); + Some(doc).filter(|it| !it.is_empty()) +} + +fn collect_comment_text(node_iter: impl Iterator) -> String { + // Take text of all immediately subsequent `TOKEN_COMMENT`s, + // skipping over whitespace-only tokens. + // Note this would be more clearly written using `map_while`, but that + // does not seem to be in Rust stable yet. + node_iter + .filter_map(|node| node.into_token()) + .take_while(|tok| tok.kind() == TOKEN_COMMENT || tok.kind().is_trivia()) + .map(|tok| tok.text().to_string()) + .map(|s| s.trim_start_matches('#').trim().to_string()) + .filter(|s| !s.is_empty()) + .collect::>() + .join("\n") +} diff --git a/ast-checks/src/common_structs.rs b/ast-checks/src/common_structs.rs new file mode 100644 index 0000000..c650afe --- /dev/null +++ b/ast-checks/src/common_structs.rs @@ -0,0 +1,16 @@ +use serde::{Deserialize, Serialize}; + +#[derive(Serialize, Deserialize, Debug)] +pub struct SourceLocation { + pub column: usize, + pub line: usize, + pub file: String, +} + +#[derive(Serialize, Deserialize, Debug)] +pub struct NixpkgsHammerMessage { + pub cond: bool, + pub locations: Vec, + pub msg: &'static str, + pub name: &'static str, +} diff --git a/ast-checks/src/missing-patch-comment.rs b/ast-checks/src/missing-patch-comment.rs new file mode 100644 index 0000000..9e360ce --- /dev/null +++ b/ast-checks/src/missing-patch-comment.rs @@ -0,0 +1,128 @@ +use codespan::{FileId, Files}; +use comment_finders::{find_comment_above, find_comment_within}; +use common_structs::{NixpkgsHammerMessage, SourceLocation}; +use rnix::types::*; +use std::{collections::HashMap, env, error::Error, fs}; +use tree_utils::walk_keyvalues_filter_key; + +mod comment_finders; +mod common_structs; +mod tree_utils; + +fn main() -> Result<(), Box> { + let args: Vec = env::args().skip(1).collect(); + let mut report = HashMap::new(); + + let mut files = Files::new(); + for filename in args { + let file_id = files.add(filename.clone(), fs::read_to_string(&filename)?); + report.insert(filename.clone(), analyze_single_file(&files, file_id)?); + } + + println!("{}", serde_json::to_string(&report)?); + Ok(()) +} + +fn analyze_single_file( + files: &Files, + file_id: FileId, +) -> Result, Box> { + let ast = rnix::parse(files.source(file_id)) + .as_result() + .map_err(|_| { + format!( + "Unable to parse {} as a nix file", + files + .name(file_id) + .to_str() + .unwrap_or("unprintable file, encoding error") + ) + })?; + let root = ast.root().inner().ok_or(format!( + "No elements in the AST in path {}", + files + .name(file_id) + .to_str() + .unwrap_or("unprintable file, encoding error") + ))?; + let mut report: Vec = vec![]; + + // Find all “patches” attrset attributes, that + // *do not* have a comment directly above them. + let patches_without_top_comment = walk_keyvalues_filter_key(&root, "patches").filter(|kv| { + // Look for a comment directly above the `patches = …` line + // (Interpreting that as a comment which applies to all patches.) + find_comment_above(kv.node()).is_none() + }); + + // For each list of patches without a top comment, + // produce a report for each patch that is missing a per-patch comment. + for kv in patches_without_top_comment { + report.extend(process_patch_list(kv, files, file_id)?); + } + + Ok(report) +} + +fn process_patch_list( + kv: KeyValue, + files: &Files, + file_id: FileId, +) -> Result, Box> { + let mut report: Vec = vec![]; + + match kv.value().and_then(List::cast) { + Some(value) => { + // For each element in the list of patches, look for + // a comment directly above the element or, if we don't see + // one of those, look for a comment within AST of the element. + for item in value.items() { + let has_comment_above = find_comment_above(&item).is_some(); + let has_comment_within = find_comment_within(&item).is_some(); + + if !(has_comment_above || has_comment_within) { + let start = item.text_range().start().to_usize() as u32; + let loc = files.location(file_id, start)?; + + report.push(NixpkgsHammerMessage { + cond: true, + msg: "Please add a comment on the line above, explaining the purpose of this patch.", + name: "missing-patch-comment", + locations: vec![SourceLocation { + file: files + .name(file_id) + .to_str() + .ok_or("encoding error")? + .to_string(), + // Convert 0-based indexing to 1-based. + column: loc.column.to_usize() + 1, + line: loc.line.to_usize() + 1, + }], + }); + } + } + } + None => { + let start = kv.node().text_range().start().to_usize() as u32; + let loc = files.location(file_id, start)?; + + report.push(NixpkgsHammerMessage { + cond: true, + msg: "`patches` should be a list.", + name: "missing-patch-comment", + locations: vec![SourceLocation { + file: files + .name(file_id) + .to_str() + .ok_or("encoding error")? + .to_string(), + // Convert 0-based indexing to 1-based. + column: loc.column.to_usize() + 1, + line: loc.line.to_usize() + 1, + }], + }); + } + }; + + Ok(report) +} diff --git a/ast-checks/src/tree_utils.rs b/ast-checks/src/tree_utils.rs new file mode 100644 index 0000000..7f59ebe --- /dev/null +++ b/ast-checks/src/tree_utils.rs @@ -0,0 +1,41 @@ +use rnix::{types::*, SyntaxElement, SyntaxKind, SyntaxKind::*, SyntaxNode, WalkEvent}; +use std::iter::{once, successors}; + +pub fn walk_kind(node: &SyntaxNode, kind: SyntaxKind) -> impl Iterator { + // Iterates over all AST nodes under `node` with the specified `kind`. + node.preorder_with_tokens() + .filter_map(move |event| match event { + WalkEvent::Enter(element) => Some(element).filter(|it| it.kind() == kind), + WalkEvent::Leave(_) => None, + }) +} + +pub fn walk_keyvalues_filter_key<'a>( + node: &SyntaxNode, + key: &'a str, +) -> impl Iterator + 'a { + // Iterates over all AST nodes under `node` that are of kind `NODE_KEY_VALUE`, + // (which corresponds to a key-value pair inside a nix attrset) and yields + // only the nodes where the key is equal to `key`. + walk_kind(node, NODE_KEY_VALUE) + .filter_map(|element| element.into_node()) + .filter_map(|node| KeyValue::cast(node)) + .filter(move |kv| { + kv.key() + .map_or(false, |e| e.node().to_string() == key.to_string()) + }) +} + +pub fn next_siblings(element: &SyntaxElement) -> impl Iterator { + // Iterates over both `element` and its sibling elements that come after it in the AST. + once(element.clone()).chain(successors(element.next_sibling_or_token(), |it| { + it.next_sibling_or_token() + })) +} + +pub fn prev_siblings(element: &SyntaxElement) -> impl Iterator { + // Iterate over both `element` and its sibling elements that come before it in the AST. + once(element.clone()).chain(successors(element.prev_sibling_or_token(), |it| { + it.prev_sibling_or_token() + })) +} diff --git a/explanations/missing-patch-comment.md b/explanations/missing-patch-comment.md new file mode 100644 index 0000000..b4c42aa --- /dev/null +++ b/explanations/missing-patch-comment.md @@ -0,0 +1,37 @@ +Each patch in nixpkgs applied to the upstream source should be documented. Out-of-tree patches, fetched using `fetchpatch`, are preferred. + +In each patch comment, please explain the purpose of the patch and link to the relevant upstream issue if possible. If the patch has been merged upstream but is not yet part of the released version, please note the version number or date in the comment such that a future maintainer updating the nix expression will know whether the patch has been incorporated upstream and can thus be removed from nixpkgs. + +Furthermore, please use a *stable* URL for the patch. Rather than, for example, linking to a GitHub pull request of the form `https://github.com/owner/repo/pull/pr_number.patch`, which would change every time a commit is added or the PR is force-pushed, link to a specific commit patch in the form `https://github.com/owner/repo/commit/sha.patch`. + +Here are two good examples of patch comments: + +```nix +mkDerivation { + … + + patches = [ + # Ensure RStudio compiles against R 4.0.0. + # Should be removed next 1.2.X RStudio update or possibly 1.3.X. + (fetchpatch { + url = "https://github.com/rstudio/rstudio/commit/3fb2397c2f208bb8ace0bbaf269481ccb96b5b20.patch"; + sha256 = "0qpgjy6aash0fc0xbns42cwpj3nsw49nkbzwyq8az01xwg81g0f3"; + }) + ]; +} +``` + +```nix +mkDerivation { + … + + patches = [ + # Allow building with bison 3.7 + # PR at https://github.com/GoldenCheetah/GoldenCheetah/pull/3590 + (fetchpatch { + url = "https://github.com/GoldenCheetah/GoldenCheetah/commit/e1f42f8b3340eb4695ad73be764332e75b7bce90.patch"; + sha256 = "1h0y9vfji5jngqcpzxna5nnawxs77i1lrj44w8a72j0ah0sznivb"; + }) + ]; +} +``` diff --git a/flake.lock b/flake.lock index 34a434c..8425204 100644 --- a/flake.lock +++ b/flake.lock @@ -16,13 +16,33 @@ "type": "github" } }, + "naersk": { + "inputs": { + "nixpkgs": [ + "nixpkgs" + ] + }, + "locked": { + "lastModified": 1611671864, + "narHash": "sha256-dufXzwMM8q0zkqSsfK2pvBNBmA8AJbaPkZkDYVCUXsI=", + "owner": "nmattia", + "repo": "naersk", + "rev": "79129b5c16d299b747b5276afd80b1f325f786ac", + "type": "github" + }, + "original": { + "owner": "nmattia", + "repo": "naersk", + "type": "github" + } + }, "nixpkgs": { "locked": { - "lastModified": 1609945281, - "narHash": "sha256-//49JhpLwwqra1bx2q5i8pI1HsGFgNmnoPtklF0uTcw=", + "lastModified": 1612101959, + "narHash": "sha256-7IsEL/6wpr1fI/XyIw26Rz+KjRCoJvoozIcTRpCFfMQ=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "44c443a7a602e7752641aac21e4ad17061761290", + "rev": "1b77b735ea70c3dfbdab3eaa79aee48d75d3e162", "type": "github" }, "original": { @@ -35,17 +55,18 @@ "root": { "inputs": { "flake-compat": "flake-compat", + "naersk": "naersk", "nixpkgs": "nixpkgs", "utils": "utils" } }, "utils": { "locked": { - "lastModified": 1609935452, - "narHash": "sha256-McwA/tAnnS8LaAdEoONBsdKFHuOpHMxKqpMCU1wL7H4=", + "lastModified": 1610051610, + "narHash": "sha256-U9rPz/usA1/Aohhk7Cmc2gBrEEKRzcW4nwPWMPwja4Y=", "owner": "numtide", "repo": "flake-utils", - "rev": "8088c6dbe86a7e6e6396c83e43020e8a1edb08d5", + "rev": "3982c9903e93927c2164caa727cd3f6a0e6d14cc", "type": "github" }, "original": { diff --git a/flake.nix b/flake.nix index fa10e98..87490da 100644 --- a/flake.nix +++ b/flake.nix @@ -7,26 +7,50 @@ flake = false; }; + naersk = { + url = "github:nmattia/naersk"; + inputs.nixpkgs.follows = "nixpkgs"; + }; + nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable"; utils.url = "github:numtide/flake-utils"; }; - outputs = { self, flake-compat, nixpkgs, utils }: utils.lib.eachDefaultSystem (system: let + outputs = { self, flake-compat, naersk, nixpkgs, utils }: utils.lib.eachDefaultSystem (system: let pkgs = import nixpkgs { inherit system; }; - in { - packages.nixpkgs-hammer = pkgs.runCommand "nixpkgs-hammer" { - buildInputs = with pkgs; [ - python3 - ]; - } '' - install -D ${./tools/nixpkgs-hammer} $out/bin/$name - patchShebangs $out/bin/$name - substituteInPlace $out/bin/$name \ - --replace "NIX_INSTANTIATE_PATH = 'nix-instantiate'" "NIX_INSTANTIATE_PATH = '${pkgs.nix}/bin/nix-instantiate'" - ln -s ${./overlays} $out/overlays - ln -s ${./lib} $out/lib - ''; + naersk-lib = naersk.lib."${system}"; + in rec { + + packages.ast-checks = naersk-lib.buildPackage { + name = "ast-checks"; + root = ./ast-checks; + }; + + packages.nixpkgs-hammer = + let + # Find all of the binaries installed by ast-checks. + ast-check-names = + let + cargo = builtins.fromTOML (builtins.readFile ./ast-checks/Cargo.toml); + in + map (bin: bin.name) cargo.bin; + in + pkgs.runCommand "nixpkgs-hammer" { + buildInputs = with pkgs; [ + python3 + makeWrapper + ]; + } '' + install -D ${./tools/nixpkgs-hammer} $out/bin/$name + patchShebangs $out/bin/$name + + wrapProgram "$out/bin/$name" \ + --prefix PATH ":" ${pkgs.lib.makeBinPath [ pkgs.nixUnstable packages.ast-checks ]} \ + --set AST_CHECK_NAMES ${pkgs.lib.concatStringsSep ":" ast-check-names} + ln -s ${./overlays} $out/overlays + ln -s ${./lib} $out/lib + ''; defaultPackage = self.packages.${system}.nixpkgs-hammer; @@ -37,7 +61,8 @@ devShell = pkgs.mkShell { buildInputs = with pkgs; [ python3 - nix + rustc + cargo ]; }; }); diff --git a/run-tests.py b/run-tests.py index 71417ea..9340213 100755 --- a/run-tests.py +++ b/run-tests.py @@ -12,7 +12,7 @@ def case(): attr_path=f'{rule}.{variant}' if variant is not None else rule test_build = subprocess.run( [ - os.path.join(script_dir, 'tools/nixpkgs-hammer'), + 'nixpkgs-hammer', '-f', './tests', attr_path ], @@ -102,6 +102,18 @@ def __iter__(self): 'meson-cmake' ) + yield make_test_rule( + 'missing-patch-comment', + [ + 'missing-comment', + ], + [ + 'general-comment', + 'comment-above', + 'comment-within', + ] + ) + yield make_test_rule( 'missing-phase-hooks', [ diff --git a/tests/default.nix b/tests/default.nix index ca38425..24d8288 100644 --- a/tests/default.nix +++ b/tests/default.nix @@ -9,6 +9,7 @@ explicit-phases = pkgs.callPackage ./explicit-phases { }; fixup-phase = pkgs.callPackage ./fixup-phase { }; meson-cmake = pkgs.callPackage ./meson-cmake { }; + missing-patch-comment = pkgs.callPackage ./missing-patch-comment { }; missing-phase-hooks = pkgs.callPackage ./missing-phase-hooks { }; patch-phase = pkgs.callPackage ./patch-phase { }; python-explicit-check-phase = pkgs.python3.pkgs.callPackage ./python-explicit-check-phase { }; diff --git a/tests/missing-patch-comment/comment-above.nix b/tests/missing-patch-comment/comment-above.nix new file mode 100644 index 0000000..5fa3e16 --- /dev/null +++ b/tests/missing-patch-comment/comment-above.nix @@ -0,0 +1,17 @@ +{ stdenv +}: + +stdenv.mkDerivation { + name = "comment-above"; + + src = ../fixtures/make; + + patches = [ + # comment for a + "a" + # comment for b + "b" + # comment for c + "c" + ]; +} diff --git a/tests/missing-patch-comment/comment-within.nix b/tests/missing-patch-comment/comment-within.nix new file mode 100644 index 0000000..ae195ce --- /dev/null +++ b/tests/missing-patch-comment/comment-within.nix @@ -0,0 +1,16 @@ +{ stdenv +, fetchpatch +}: + +stdenv.mkDerivation { + name = "comment-within"; + + src = ../fixtures/make; + + patches = [ + (fetchpatch { + # comment within + url = "bla-bla-bla"; + }) + ]; +} diff --git a/tests/missing-patch-comment/default.nix b/tests/missing-patch-comment/default.nix new file mode 100644 index 0000000..c5fc6e9 --- /dev/null +++ b/tests/missing-patch-comment/default.nix @@ -0,0 +1,12 @@ +{ pkgs +}: + +{ + # positive cases + missing-comment = pkgs.callPackage ./missing-comment.nix { }; + + # negative cases + general-comment = pkgs.callPackage ./general-comment.nix { }; + comment-above = pkgs.callPackage ./comment-above.nix { }; + comment-within = pkgs.callPackage ./comment-within.nix { }; +} diff --git a/tests/missing-patch-comment/general-comment.nix b/tests/missing-patch-comment/general-comment.nix new file mode 100644 index 0000000..e6591df --- /dev/null +++ b/tests/missing-patch-comment/general-comment.nix @@ -0,0 +1,15 @@ +{ stdenv +}: + +stdenv.mkDerivation { + name = "general-comment"; + + src = ../fixtures/make; + + # global comment + patches = [ + "a" + "b" + "c" + ]; +} diff --git a/tests/missing-patch-comment/missing-comment.nix b/tests/missing-patch-comment/missing-comment.nix new file mode 100644 index 0000000..f0c05b4 --- /dev/null +++ b/tests/missing-patch-comment/missing-comment.nix @@ -0,0 +1,14 @@ +{ stdenv +}: + +stdenv.mkDerivation { + name = "missing-comment"; + + src = ../fixtures/make; + + patches = [ + "a" + "b" + "c" + ]; +} diff --git a/tools/nixpkgs-hammer b/tools/nixpkgs-hammer index 7217c58..10fa265 100755 --- a/tools/nixpkgs-hammer +++ b/tools/nixpkgs-hammer @@ -1,14 +1,62 @@ #!/usr/bin/env python3 +import os from pathlib import Path import argparse import json import subprocess import sys import textwrap +from typing import Dict, List +from collections import defaultdict -NIX_INSTANTIATE_PATH = 'nix-instantiate' +def get_ast_check_programs(): + # Each rule that is implemented as an AST check rather than an overlay + # is installed onto our PATH, and the names of the rules put into this + # environment variable. + return set(os.environ['AST_CHECK_NAMES'].split(':')) + + +def attr_to_file_path(attr: str, nix_file: str) -> str: + env = dict(os.environ) + env['EDITOR'] = 'echo' + + proc = subprocess.run( + ['nix', 'edit', '--experimental-features', 'nix-command', '-f', nix_file, attr], + env=env, + stdout=subprocess.PIPE, + text=True, + check=True, + ) + return proc.stdout.strip() + + +def ast_checks(nix_file: str, attrs: List[str], excluded_rules: List[str]) -> Dict[str, List]: + file_to_attr = defaultdict(list) + for attr in attrs: + file = attr_to_file_path(attr, nix_file) + file_to_attr[file].append(attr) + + rules = get_ast_check_programs() - set(excluded_rules) + + result = defaultdict(list) + for rule in rules: + cmd = [rule] + list(file_to_attr.keys()) + proc = subprocess.run(cmd, stdout=subprocess.PIPE, check=True, text=True) + output = json.loads(proc.stdout) + for file, report in output.items(): + for attr in file_to_attr[file]: + result[attr].extend(report) + return dict(result) + + +def concatenate_messages(*message_sources): + result = defaultdict(list) + for m in message_sources: + for attr, report in m.items(): + result[attr].extend(report) + return result def escape_nix_string(val: str) -> str: @@ -16,7 +64,7 @@ def escape_nix_string(val: str) -> str: def nix_eval_json(expr: str, show_trace: bool=False): - args = [NIX_INSTANTIATE_PATH, '--strict', '--json', '--eval', '-E', expr] + args = ['nix-instantiate', '--strict', '--json', '--eval', '-E', expr] if show_trace: args.append('--show-trace') @@ -155,7 +203,9 @@ def main(args): if args.show_trace: print('Nix expression:', all_messages_nix, file=sys.stderr) - all_messages = nix_eval_json(all_messages_nix, args.show_trace) + all_overlay_messages = nix_eval_json(all_messages_nix, args.show_trace) + all_ast_check_messages = ast_checks(args.nix_file, args.attr_paths, args.excluded_rules) + all_messages = concatenate_messages(all_overlay_messages, all_ast_check_messages) if args.json: print(json.dumps(all_messages)) @@ -171,6 +221,7 @@ def main(args): if __name__ == '__main__': parser = argparse.ArgumentParser( + prog='nixpkgs-hammer', description='check package expressions for common mistakes', ) parser.add_argument(