Avoid rec expresions in nixpkgs?

As @domenkozar mentions here: https://nix.dev/anti-patterns/language.html#rec-expression, rec expressions are an anti-pattern. I have been bitten by rec expressions trying to customize some derivations with overrideAttrs.

But rec expression are used a lot in nixpkgs. Should we try to replace them with a let when updating/creating derivations on nixpkgs?

e.g. replacing

{ stdenv, fetchurl }:

stdenv.mkDerivation rec{
  pname = "foo";
  version = "1.0";

  src = fetchurl {
    url = "https://foo-${version}.tar.gz";
    sha256 = "...";
  };
}

with

{ stdenv, fetchurl }:
let
  version = "1.0";
in
stdenv.mkDerivation {
  pname = "foo";
  inherit version;

  src = fetchurl {
    url = "https://foo-${version}.tar.gz";
    sha256 = "...";
  };
}
1 Like

I understand the argument, recursive attrsets can lead to confusion. Also someone said (I don’t remember who) that recursive attrsets are quite a bit more expensive to evaluate than non-recursive attrsets.

That said, in the case of derivations I have to say I don’t really like the reduced readability.

At any rate, I think the discussion can easily lead to bikeshedding. But it would be great to have some guideline (both are acceptable / we prefer non-recursive attrsets / we prefer recursive attrsets for simple derivations) to encourage uniformity in derivations.

6 Likes

At any rate, I think the discussion can easily lead to bikeshedding

I totally agree with this. One thing I really don’t want to see are review comments mentioning this (and therefore delaying PRs unnecessarily while we have an overloaded queue of patches waiting to get reviewed/merged).

If someone opens a PR doing a bulk-replace for hand-written drvs it should be fine. Another idea might be to implement a simple rule for nixpkgs-fmt (which moves pname/version into a let ... in block at least) to incrementally change this.

3 Likes

There are a couple of pitfalls:

  • It’s possible to introduce a hard to debug error infinite recursion when shadowing a variable, the simplest example being rec { b = b; } .
  • combining with overriding logic such as overrideAttrs function in nixpkgs has a suprising behavour of not overriding every reference.

While I agree that rec attrsets suffer from the issues mentioned, I would not migrate backÂą to let expression, at least for things like version.

I would argue that it is quite unlikely that simple attribute like version would cause an infinite recursion – it would be a literal value most of the time. So of the two mentioned problems, only the second one would be relevant here. And while let expression makes it explicit that the value is not overridable when you see the expression, I would expect most people to encounter the issue when calling pkg.overrideAttrs (attrs: { version = "foo"; }) without inspecting the package – that would not work with let either.

Therefore, let expression does not solve either of the aforementioned pitfalls for your example.

There is a pattern that allows using overrideAttrs:

{ stdenv, fetchurl }:
let self = stdenv.mkDerivation {
  pname = "foo";
  version = "1.0";

  src = fetchurl {
    url = "https://foo-${self.version}.tar.gz";
    sha256 = "...";
  };
}; in self

But I am not sure the decreased legibility is worth it. Also it does not currently work with python.pkgs.buildPythonPackage.

Of course, for complex values, narrowly scoped let expressions should be preferred but I would avoid calling rec an antipattern.


Âą We used to define version using let expression quite a lot, but when mkDerivation started accepting pname and version, instead of just name, we started passing them to the derivation directly to reduce verbosity.

12 Likes

It would be nice if Nix had a shorthand syntax equivalent to let ident = …; in ident. rec { … } could then be replaced with this, and this would make self references very obvious. I’d suggest self@{ … } except that’s already used for function arguments, and I expect there are parsing issues between allowing self@expr and self@{a, b} (as function arguments aren’t an expression), in addition to the fact that the function argument form allows { … }@foo and expr@foo isn’t a good idea (and would be ambiguous with foo@bar).

Of course, this doesn’t solve the overrideAttrs issue, and shorthand like self@(stdenv.mkDerivation { … }) is ugly, though perhaps less so than let self = stdenv.mkDerivation { … }; in self.

Though come to think of it, ident@(expr) would be an unambiguous form for this, and would also avoid any issues of precedence (e.g. self@foo + bar).

1 Like

Technically you’d actually want to use self.drvAttrs.version, otherwise defining passthru.version would screw with this.


Another idea is to allow stdenv.mkDerivation to take a function instead of an attrset, where it would pass the attributes to the function. This would then change that to

stdenv.mkDerivation (self: {
  pname = "foo";
  version = "1.0";

  src = fetchurl {
    url = "https://foo-${self.version}.tar.gz";
    sha256 = "…";
  };
})

With this approach overrideAttrs would change the self value passed here, which would allow overriding of version to work.

I’m inclined to say this should pass the mkDerivation attrs to the function rather than the resulting derivation, to cut down on the inclination to do stuff that might cause recursion and to make it much closer to the current rec approach.

This is also strictly additive, as mkDerivation can detect whether it’s given a function. overrideAttrs wouldn’t change its external behavior. Though there is the risk of causing recursion with overrideAttrs here, such as with

(stdenv.mkDerivation (attrs: {
  name = "foo-${attrs.version}";
  version = "1.0";
  …
}).overrideAttrs (old: {
  version = "${lib.getVersion old.name}-beta";
})

but this is probably pretty low-risk (any code like this has to understand the structure of the original derivation’s attributes anyway).

A prototype of the new approach would look like

let
  mkDerivation = attrs:
    if builtins.isFunction attrs
    then lib.extendDerivation true {
      overrideAttrs = f: mkDerivation (self: let super = attrs self; in super // (f super));
    } (stdenv.mkDerivation (lib.fix attrs))
    else stdenv.mkDerivation attrs;
in
  …
3 Likes

I guess another, simpler, option would be to take version and sha256 as derivation arguments. This would even allow the user to use override if they only want to retrieve a different version and it doesn’t require other changes:

{ stdenv
, version ? "1.2.3"
, sha256 ? "somehash"
}:

stdenv.mkDerivation {
  inherit version;
  pname = "foobar";

  src = fetchurl {
    inherit sha256;
    url = "https://example.org/foobar-${version}.tar.gz";
  };

  # ...
}
7 Likes

My fear with this approach is this suggests that the version is officially supported as a changeable parameter and that the derivation will do the right thing when you pass different versions. But the derivation doesn’t actually know how to support any other version than what it’s defined with (other versions may require different dependencies or tweaks to build steps). Using overrideAttrs makes it a lot more clear that the caller is responsible for ensuring the resulting derivation is correct.

4 Likes

From reviewing version bump PRs, it seems to me that for many packages this would just work.

I think it is preferable for newcomers to have something that works in 90% of the cases (with a warning about the 10% of the cases that don’t work) than something which is hard to understand to anyone who is not familiar with the Nix language or nixpkgs conventions.

I guess the onus is on me to prove that 90% of the version bumps are simple :stuck_out_tongue: .

2 Likes

I stand corrected: I wrote a one-off script that checks diffs of version bumps (->) whether only hashes and versions are updated and checked the last 1000 version bump commits. ~75% are trivial bumps. In the remaining 25% there are also changes that wouldn’t affect version overrides, but there are definitely also a lot of changed dependencies, custom patches. etc.

2 Likes

I experimented with this a bit more, and realized that it’s actually a little awkward to override the sha256 in most src expressions. In particular, neither fetchurl nor fetchFromGitHub offer a way to override the arguments given to the fetch function (override affects the dependencies available to the fetcher itself, overrideAttrs affects the resulting derivation), so you have to rely on the fact that the arg will end up as outputHash and isn’t used otherwise. That said, my proposed modified mkDerivation makes it a little simpler, e.g. it turns

hello.overrideAttrs (old: {
  version = "2.9";
  src = fetchurl {
    url = "mirror://gnu/hello/hello-2.9.tar.gz";
    sha256 = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  };
  meta = old.meta // {
    changelog = "https://git.savannah.gnu.org/cgit/hello.git/plain/NEWS?h=v2.9";
  };
})

into

hello.overrideAttrs (old: {
  version = "2.9";
  src = old.src.overrideAttrs (_: {
    outputHash = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  });
})

Note how in the former I needed to reproduce the entire src expression in order to provide an updated url; in the latter I just need to override outputHash.

2 Likes

I went ahead and filed a draft PR with my proposal from this thread.

1 Like

The moment a software called version gets added to all-packages.nix, it’ll break just as much as having src in the attributes to your derivation without actually passing it.

EDIT: or worse! What if version = lib.version; becomes a thing!

1 Like

Sure. But that’s currently true for any non-package argument, and we have a wide variety of them. Also this would be caught rather quickly, because IIRC stdenv already checks that version is a string.

Right, though this actually brings the point (that I didn’t know that) I wanted to bring: the callPackage way of doing dependency injection has an overload of responsibility that is probably fatal.

There is no way to mark a dependency as a derivation vs. a configuration option.

In that sense, my opinion (and, that’s like, my opinion) is that we should reduce overuse of the dependency injection in callPackage for non-packages. Though, I’m sure this opinion is not universal, and understandably :).

2 Likes

I forgot who actually proposed this, but someone argued (IIRC a year or so ago) that derivation functions in nixpkgs should take two attrset arguments – one for dependencies and one with configuration options. I think another downside of the current approach is that configuration options of packages are not really discoverable (though two attrsets wouldn’t really solve that).

Though there are probably more principled solutions.

4 Likes