Defining variables with default values in package arguments can lead to problems

[Aleksanaa]

Problem

To make overriding more feasible, people often write something like

{
  sth,
  doCheck? true,
  patches? [],
  foo? callPackage ./foo.nix,
}:

{ ... }

But this is actually dangerous. We don't know when a top-level package named "doCheck", "patches" or "foo" will appear. While the first two can easily be found on ofBorg failures, the third might be quietly passed, causing more problems (and it even looks unrelated!).

Reproduce

# default.nix
{ aaa? "1" }:

aaa

This should be "1", right?

=> nix eval --impure --expr 'with import <nixpkgs> {}; callPackage ./default.nix {}' 
«derivation /nix/store/l36n95rwc21rn1f8sld428j3sarv3ssd-aaa-1.1.1.drv»

Well it's not.

Solution

My idea is that we set some "reserved attribute prefixes", such as names starting with "var-", and make sure that they cannot be used for top-level package names. We may also need to take the same approach for other sets that define callPackage.

[nim65s]

Prefixing those variables with pname could also be an option. For example, in package zblarg, one could probably use zblargDoCheck, zblargPatches, zblargWithPython and zblargAaa with a minimal risk of a new unrelated package being named that.

It would also be more explicit, as zblargWithPython and fooWithPython would be different, while var-withPython and var-withPython in packages zblarg and foo wouldn't be.

And I think this is a common enough practice in various language and build systems already, so this wouldn't be too hard to adopt here.

[TomaSajt]

An example for when I've encountered this unexpected default populating logic was when taking in an src value (it turns out it's also the name of a VCS package)

[reckenrode]

Does NixOS/rfcs#169 at least partially address this issue?

[jtojnar]

I would say that if you need to pass doCheck, src, patches or other mkDerivation arguments, you are not calling a package but a package builder/factory, so you should not use callPackage. And if your goal is to be overridable, those can be overridden nicely with overrideAttrs (assuming finalAttrs pattern) without having to stuff them into package expression arguments.

And if you are doing that trying to follow DRY or something, that is just more mess. Especially since using callPackage inside callPackage will break override anyway #311780 (comment)

The boolean/enum feature parameters are also ugly but at least they typically follow consistent naming pattern that is unlikely to conflict.

Lastly, as for internal packages, it might be fine if they are not overridable. They are often temporary or tied to the main derivation closely enough that the need to override them should be rare.

Though if we really want to, …

we could create a private scope and call the package from within – something like the example below. Then it would be just possible to use override like with any other package mainProgram.override (old: { internalLib = old.internalLib.overrideAttrs …; })

pkgs/top-level/all-packages.nix

# …
my-program = import pkgs/applications/my-program { inherit pkgs lib; };
# …

pkgs/applications/my-program/default.nix

{ pkgs, lib }:

let
  privateScope = lib.makeScope pkgs.newScope (self:
    let
      inherit (self) callPackage;
    in {
      internalLib = callPackage ./foo {};
      # depends on internalLib
      mainProgram = callPackage ./prog {};
    }
  );

in
privateScope.mainProgram

[bendlas]

I would say that if you need to pass doCheck, src, patches or other mkDerivation arguments, you are not calling a package but a package builder/factory, so you should not use callPackage.

So if you're doing a factory, but you still need a couple of inputs, that you'd rather not pass through, what would you recommend instead of auto-wiring with callPackage? Passing pkgs and selecting the dependencies yourself?

Is there room for a callPackage variant, that doesn't inject arguments, that have defaults?

Arguably in hindsight, wouldn't that have been a better behavior for callPackage in the first place?

[nixos-discourse]

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

https://discourse.nixos.org/t/override-derivation-with-overrideattrs-any-way-to-handle-let-in/47307/2

[Aleksanaa]

So we have an unfortunate example now: #324199 uses sources? callPackage ./sources.nix, which broke @flokli 's configuration (which includes sources as a top-level attribute). This is reverted in #325242.

I didn't initially realize that top-level attributes introduced by users through overlays might also affect our compatibility.

[flokli]

I extended the docs for pkgs/by-name to discourage being more fancy than simple enable* booleans for customization (other than package inputs without defaults) in #325262.

[fgaz]

Another idea that would also help remove splicing but requires extending the nix language:

{ lib # ← could be in a "platformIndependent" set instead of top level
, sources
, options =
    { enableFoo ? true
    , enableBar ? false
    }
, buildHost =
    { stdenv
    , cmake
    , sometool
    }
, hostTarget =
    { libfoo
    , libbar
    }
}:
stdenv.mkDerivation ...

[AndersonTorres]

Does NixOS/rfcs#169 at least partially address this issue?

In a certain sense yes. @Atemu was working on a module system.

[Atemu]

if your goal is to be overridable, those can be overridden nicely with overrideAttrs (assuming finalAttrs pattern) without having to stuff them into package expression arguments.

I take issue with this. overrideAttrs is all but well-defined. It's like a screw driver vs. a scalpel. One is a rather imprecise high-level tool that provides standardised operation while the other is a precision tool for an entirely different purpose.

I converted ffmpeg to a new override API a while back where the source, version etc. are intentionally defined in the function parameters: #295691 (While doing so I also ran into the same issue @TomaSajt ran into.)
This is necessary because the with*/enable* parameters have conditional defaults which depend on the version attr. Using overrideAttrs to override version did not change these parameters and caused annoying incompatibilities.

This method explicitly exposes the src/"source" as an input as part of the drv's API which allows you to transform any ffmpeg derivation into any other by merely adjusting inputs. Our ffmpeg variants are also defined through the same mechanism; setting overrides at callPackage site.

This has proven to work well and have some very nice properties. Therefore I'd like propose the exact opposite to @flokli: Reserve names that are likely to be part of package APIs such as src, version, pname or hash to not be used for package names. Obviously we shouldn't expose every mkDerivation arg in this way (that's what overrideAttrs is for) but things that can be considered as part of a package's "outside API" and that can very well include version or src in case of a generic drv IMHO.

Another idea that would also help remove splicing but requires extending the nix language:

This has been discussed in RFC 169.

In a certain sense yes. @Atemu was working on a module system.

#312432

It's actually quite simple to implement and upgrade existing flags defined as drv override inputs to a module system interface.

IMHO it quite neatly solves this problem and one of the primary problems RFC 169 intends to resolve or at least removes quite a lot of limitations around it.

We will need to figure out a way to measure the performance impact of such a module system before we can apply it more broadly. This is quite tricky as we'd need to actually convert override flags to mini module systems in order to measure the impact of it being used more widely.

One idea I had on this would be to create an overlay which would generate a small module system for each package and force their eval by injecting their results via overrideAttrs. This would represent be the worst-case scenario where each package has configurable parameters which obviously won't be the case but should show potential non-linear scaling.

[fgaz]

Another idea that would also help remove splicing but requires extending the nix language:

This has been discussed in RFC 169.

Could you provide a link to the discussion? The rfc page is long and there are a lot of collapsed threads

[infinisil]

Please check out this previous PR of mine, that was essentially done and could address this problem:

• Enforce that package argument defaults are applied, cleans up optional dependency convention #131271