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?

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.

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))?

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.

Can you use StateDirectory= in the unit?

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.

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.

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?

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

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).

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.

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