nixos/hercules-ci-agent: init

[roberth]

Motivation for this change

Make Hercules CI Agent easier to install and configure.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[flokli]

I'm not a fan of something failing at build time here.

What about checking the nix version and failing if it's too low at evaluation time, and prompting users to use nixUnstable in that case?

[roberth]

I would like to avoid depending on unreleased versions, which is why I've created the option to apply a minimal patch on the released version.
Deriving this information from options only seems a bit risky. For example if the user has their own patched Nix, how do we check that it's ok?

It's only a temporary solution. Once 2.4 is released, I'm happy to reduce this to a simple eval-only check.

[flokli]

Hmmh, this code also depends on unreleased versions. Contrary to nixUnstable, you build your own version on-the-fly here, which is even less risky for sudden build failure.

I'd assume Nix 2.4 will be released before 19.10 - mind just recommending nixUnstable in an evaluation failure/warning (depending on how required it is) here if !(lib.versionAtLeast (lib.getVersion config.nix.package))?

[roberth]

The patch has been tested since 2.3.4 and has not needed any changes for the total of four releases. It's a patch on the stable branch after all.
I'd be happy to remove the patching when 2.4 is released. But I wouldn't just assume it's released before NixOS.

[flokli]

Can you use StateDirectory= in the unit?

[roberth]

This maps to an entry in the config file. I suppose I could derive StateDirectory from it, but I'm not sure what would be the benefit of doing so.

[flokli]

Hmm, I wasn't specific here - if this were using DynamicUser (which it can't, due to suid support), We could just have defaulted the state directory to this location.

It's probably still beneficial to distinguish between the home directory of the user running the service (which could be somewhere else than /var/lib/buildkite-agent), and state files (which should always live in /var/lib/$serviceName.

[roberth]

It currently depends on dotfiles to configure automatic S3 push for example, so the default home /var/empty wouldn't work. Disallowed or transient dotfiles are normally the benefit I suppose. Is there another benefit that could offset the migration that will be required for current installations?

[roberth]

Relevant issue: hercules-ci/hercules-ci-agent#239
For now I don't see a reason to use StateDirectory.

[aanderse]

This seems to break a number of conventions, deviating from accepted NixOS module standards. @flokli mentioned a number of these and I wonder if this module is more appropriate for NUR instead. If there is motivation to get this upstream (which I think is a good idea, this module provides high value) I think the comments @flokli left are important.

[roberth]

I've made the changes to turn this into a conventional module.
This now also uses RFC 42-style settings with freeform modules (thanks @infinisil !)

[flokli]

Thanks, this now looks way more appropriate :-)

I still think the "patching nix if required" part doesn't belong into the module - raise an erorr if the version of Nix isn't recent enough (or grep the header files if the version string isn't enough).

[infinisil]

Lovely to see some nice use of the recent module additions!

[infinisil]

It might be better to use an overlay for this instead, because it will allow users to still specify e.g. nix.package = pkgs.nixFlakes, and the overlay can patch all of pkgs.{nix,nixUnstable,nixFlakes} (if necessary for each of them).

[roberth]

It's only necessary for the daemon and only when its 2.3. They can set it to nixFlakes just fine because that version does not need patching.
It's a temporary situation until 2.4 is released. At that point, I'll drop the whole patching business, so there's little point in improving this.

[infinisil]

Ah so the intended flow is:

• User enables module
• Because checkNix = true by default and patchNix = false, user gets error that the Nix check failed and they need to use a version with a patch
• User sets patchNix = true, therefore overriding the default Nix version (or I guess it would error if the user also assigned nix.package in their config)

I guess that's fine

[roberth]

@flokli @aanderse @infinisil

I have addressed all comments.

The patching options are useful until 2.4 is released, so I suggest they stick around but only until then.
Any final comments?

[infinisil]

Looks good to me! The checking and patching is indeed a bit gnarly, but I think that's fine for a temporary convenience improvement