[RFC 140] Simple package paths, part 1a: Checking tool

[infinisil]

Description of Changes

Note
This was reviewed by and merged on 2023-08-29 with the Nixpkgs Architecture Team and others that joined the meeting!

This is part of the implementation of accepted RFC 140. This is split off from #237439 in order to make the CI implementation faster and easier.

In particular, this PR does two things:

• Adds a CLI tool that can check the validity of Nixpkgs according to RFC 140:

  ❯ nix-build -A tests.byName
  /nix/store/l81h9spf2h0b84g4h0n15mv92d4i27m1-nixpkgs-check-by-name
  ❯ result/bin/nixpkgs-check-by-name --help
  Program to check the validity of pkgs/by-name
  
  Usage: nixpkgs-check-by-name <NIXPKGS>
  
  Arguments:
    <NIXPKGS>  Path to nixpkgs
  

  Notably this is only exposed under tests, it's not intended to be stable.
• Ensures that the CLI tool will always be successfully built in the nixpkgs-unstable channel.
  This allows [RFC 140] Simple package paths, part 1b: Enabling the directory structure #237439 to then just fetch the tool from the nixos-* channels (potentially with some pinning inbetween), without having to do any compilation, allowing it to check all PR's without any build delay, even ones that would cause mass rebuilds.
  Notably this only works because the tool only needs to do a minimal Nix evaluation, which can run in --readonly-mode (therefore not even writing anything to the store).

Things done

• Tests
• Contributor documentation
• Clean code

[infinisil]

The code is fairly clean now, I added a bunch of comments, there's contributor docs and there's a bunch of tests, so this is ready to be reviewed!
I know it's a bit short on time, but it would be really cool if we can get this reviewed and merged next week, this would give us a good timeline to have RFC 140 Part 1 done for Nixcon 🚀

[cafkafk]

Generally code is high quality, these suggestions are mostly superficial, so feel free to resolve them if you disagree with anything. (I didn't look at nix code, only rust and markdown)

[infinisil]

Thanks for the suggestions, all applied now, also added some more code comments.

[zmitchell]

This could use a docstring explaining the purpose of the extra_nix_path parameter and how to interpret the boolean return value.

[infinisil]

Added! Also renamed the parameter to eval_accessible_paths: Vec<&Path> to be more self-explanatory.

[zmitchell]

Nitpick: generally you don't see impl blocks split like this unless there's generic type parameters or trait bounds requiring you to do so.

[infinisil]

Yeah, I was thinking that too, but I intentionally split it to have a better grouping: The long initialization vs the static utility functions merely related to the type. I'd prefer to leave it like this unless there's a good reason not to do that

[zmitchell]

Detecting errors by checking whether this writer is empty is an elegant solution, but it may not be the most obvious one. Rather than coming up with a different solution you could probably make this more obvious with a more explicit name like error_writer and a comment explaining that this writer will only be empty if no errors were encountered while parsing nixpkgs.

[infinisil]

(there is a comment below), but I renamed it to error_writer, also changed check_nixpkgs to only take an io::Write, internally wrapping it with ErrorWriter. Makes more sense that way given that it returns whether any errors were written itself.

[zmitchell]

I'm not sure what level of rigor you're looking for, but if you split this into a shard-handling layer and a package-handling layer you could property test this with proptest. The strategy (pun intended) would look something like this:

// inside test module

pub enum EntryType {
    File,
    Dir,
}

pub pkg_by_name_entry() -> impl Strategy<Value = (EntryType, String)> {
    (prop_oneof![
        EntryType::Dir,
        EntryType::File,
    ],
    <strategy generating strings>
    )
}

pub fn pkgs_by_name_entries(max_len: usize) -> impl Strategy<Value = Vec<(EntryType, String)>> {
    proptest::vec(pkgs_by_name_entry(), 0..max_len)
}

pub fn create_entries(base_dir: PathBuf, entries: Vec<(EntryType, String)>) {
    for (etype, name) in entries.iter() {
        let path = base_dir.join(name);
        use EntryType::*;
        match etype {
            Dir => <mkdir path>,
            File => <touch path>,
        }
    }
}

proptest!  {
    #[test]
    fn validates_shards(entries in pkgs_by_name_entries(100)) {
        let temp_dir = ...;
        create_entries(temp_dir, entries);
        # do some testing
        # delete temp_dir
    }
}

[infinisil]

Thanks for the suggestion! I think this is a bit overboard, I'm fairly happy with the existing integration tests in ./tests, which do at least cover all of the potential errors. And the checking isn't so complicated that I think we can benefit a lot from property tests. Especially since it seems pretty hard to come up with beneficial ones. So I'd like to leave it at that for now.

[zmitchell]

Another point on this writer. If you wanted to make things more pure, you could separate out the various operations that could write error messages, then pass in a separate writer (doesn't need to be anything more complicated than a Vec) to each one. After each operation you would std::io::copy from the Vec to the main writer that actually does the printing.

You're obviously doing more I/O this way, but the tradeoff is that you don't have to pass around the same stateful object (writer) through the entire program and you could then write unit tests ensuring that error messages actually get written for various error conditions (e.g. in a test you would supply an empty Vec to the operation under test, run the operation, then check that writer.empty == false).

[infinisil]

I'm not sure what you mean here. All of the functions already take a writer as an argument already, and I do pass a Vec in the tests to make sure each individual problem gives the expected error. I guess the tests could be for individual checks for better unit-testing, though I don't think it's very necessary here, since it's not a library meant to be re-used. Only the resulting CLI that will be used, so having the integration tests in ./tests seem good enough to me.

[infinisil]

In addition to addressing @zmitchell's comments, I also added some more code comments and this section in the README.md describing the validity checks performed, such that nobody needs to look at the RFC itself for those.

[roberth]

Reviewed

• Nix files (non-test)
• User docs

[roberth]

I don't think the package should be hidden, and it should be named as a proper package in snake case. Could you add it to all-packages.nix or is that planned for PR 1b?

[infinisil]

It's not strictly hidden, you can build it using nix-build -A tests.nixpkgsCheckByName. But it's not meant to be installed by users or to be stable, so I don't think it should be in the main package set. In particular also, tests is not being recursed into by nix search/nix-env/search.nixos.org (tests.recurseForDerivations is not set), so it doesn't show up in searches.

Changing the attribute to nixpkgs-check-by-name though sounds okay (now done).

[infinisil]

I now updated this to ensure the tool is always available pre-built on the nixos-unstable and nixos-XX.YY channels (instead of just the nixpkgs-unstable channel), which will allow CI for PR's on all development branches master and release-XX.YY to use the corresponding pre-built tooling from the channels.

[Artturin]

Suggested change

	["nixpkgs.tests.nixpkgs-check-by-name.x86_64-linux"]
	(onSystems ["x86_64-linux"] "nixpkgs.tests.nixpkgs-check-by-name")

[infinisil]

Thanks, applied!

[tomberek]

As reviewed in NAT meeting.

[infinisil]

We looked at this together in the meeting today with the NAT (except @roberth) and fixed some last things including:

• Making it work on Darwin (thanks @YorikSar, @whentze, @tomberek)
• Fix the CODEOWNERS file
• Minor change to the Hydra jobset spec (thanks @Artturin)

There was general approval to merge it, so let's go 🚀

[github-actions]

Successfully created backport PR for release-23.05:

• [Backport release-23.05] [RFC 140] Simple package paths, part 1a: Checking tool #252154

[infinisil]

This ironically blocks channel updates because it includes case-sensitive duplicate packages, which is exactly the point, we want to test that! 😄

I'm looking into fixing this :)

Edit: #252210 fixes this

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-05-nixpkgs-architecture-team-meeting-43/32635/1