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.
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.
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.
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).
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
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
…
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:
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.
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 .
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.
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
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.
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!
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 :).
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.
Now PR 119942 is merged and we have stdenv.mkDerivation supporting recursive attributes, but most existing builders still doesn’t have such support.
Technical challenge to port this support including the loss of the set pattern containing the input arguments, and large diffs including indentation changes.
In PR 234651, I introduced lib.extendRecursiveBuilder as a unified approach to define a new builder with recursive attributes support from an existing one.
{ lib
, stdenv
}:
lib.extendRecursiveBuilder stdenv.mkDerivation [ ] (finalAttrs:
{ preferLocalBuild ? true
, allowSubstitute ? false
# Some other input arguments
, ...
}@args:
removeAttrs [
# attributes not to be passed into the builder
] args // {
inherit preferLocalBuild allowSubstitute;
# Some other build process
})
I have made it to add recursive attributes support to buildPythonPackage and buildPythonApplication.
.