Superfluous use of finalAttrs.{pname,version} introducing overriding traps

[oxalica]

Describe the bug

There are many code adopting finalAttrs to reuse variables, rather than provide an overridable option. Specifically, using finalAttrs.version and finalAttrs.pname in src = is incorrect since overriding them will NOT cause it to automatically use the source from new name/version, because the hash literal is still unchanged. It will be a silent incorrect behavior when the FOD is cached locally (reusing the old one), and will be a hash-mismatch error if it's not, both causing a hard-to-debug trap.
There are also usages in build script like cp -r ${finalAttrs.pname}-${finalAttrs.version}/* $out, which is still incorrect for .overideAttrs { pname = "foo-patched"; } use case.

mkDerivation (finalAttrs: {
  version = "1.2.3";
  src = fetchFromGitHub {
    repo = finalAttrs.pname; # so adding a `-patched` suffix would try to use another repo?
    rev = "v${finalAttrs.version}"; # oops, changing version will not fetch the new src, because of unchanged hash.
    hash = "..some literal hash..";
  };
  # So overriding either `pname` or `version` makes it fail to build.
  installPhase = ''
    cp -r ${finalAttrs.pname}-${finalAttrs.version}/* $out
  '';
})

A quick search shows that there are even more incorrect usages than correct usages on the first glance:

• finalAttrs.version: https://github.com/search?q=finalAttrs.version&type=code
• finalAttrs.pname: https://github.com/search?q=finalAttrs.pname&type=code

Expected usage

Users should always override src manually, thus making a overridable illusion via finalAttrs is more misleading than not supporting it at all.

mkDerivation rec {
  pname = "some-pkg";
  version = "1.2.3";
  src = fetchFromGitHub {
    repo = "some-pkg"; # or `pname`
    rev = "v1.2.3"; # or `"v${version}"`
    hash = "..some literal hash..";
  };
  installPhase = ''
    cp -r some-pkg-*/* $out
    # Or cp -r ${some-pkg}-*/* $out
  '';
}

It seems the even the good old rec is more correct for the pname usages because it's fixed at parse time and will not be overridden anyway, which is the intended behavior for repo and installPhase. 🤔

Inspired by #277994. cc @AndersonTorres

---

Add a 👍 reaction to issues you find important.

[kirillrdy]

@oxalica in description

.overide { pname = "foo-patched"; }

i think you mean

.overrideAttrs { pname = "foo-patched"; }

[AndersonTorres]

1. repo = pname; is an anti-pattern, and finalAttrs.pname is another.
2. Reuse variables to avoid rec anti-pattern is a legitimate use of finalAttrs. The other solution is to use a self-referent attribute set, but many people believe it is too ugly...
3. src needs to be overridden. The other solution is to pass src as an input, but I don't know why this is not popular enough in Nixpkgs.

[Aleksanaa]

It will be a silent incorrect behavior when the FOD is cached locally (reusing the old one), and will be a hash-mismatch error if it's not, both causing a hard-to-debug trap.

This is more of a design flaw in hash name itself. It will happen during override and update. See NixOS/rfcs#171

[Scrumplex]

I would be interested in some kind of function based src attribute.

Something like this:

stdenv.mkDerivation {
  pname = "foo";
  version = "1.2.3";

  srcHash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="

  srcFor = { version, hash, ... }: fetchFromGitHub {
    owner = "foobar";
    repo = "foo";
    rev = "refs/tags/${version}";
    inherit hash;
  };
};

stdenv.mkDerivation {
  pname = "foo";
  version = "1.2.3";

  srcRev = "f853c074c86ce58df79fab3cfb80ae69133a31e1";
  srcHash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="

  srcFor = { rev, hash, ... }: fetchFromGitHub {
    owner = "foobar";
    repo = "foo";
    inherit rev hash;
  };
};

Where mkDerivation will assign src like this:

  src = attrs.src or srcFor = {
    inherit (finalAttrs) version;
    inherit (stdenv) system;
    rev = finalAttrs.srcRev or null;
    hash = finalAttrs.srcHash;
  };

A bigger shortcoming of this would be that it only works for derivations that need a single source.

[AndersonTorres]

in my humble opinion, src should be an input:

{lib, stdenv, src, . . .}:

[Scrumplex]

I think the root cause for this issue is that maintainers do not want to repeat the version number for src and version attributes. But src.rev and version are usually not identical either (i.e. refs/tags/x.y.z vs. x.y.z).

Maybe we can try to embed version numbers into src directly, use src as an input and then set inherit (src) version; in the derivation.

Example:

{
  stdenv,
  fetchFromGitHub,
  src ? fetchFromGitHub {
    owner = "foobar";
    repo = "foo";
    version = "1.2.3";
    tagPrefix = "v";
    hash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
  },
}:
stdenv.mkDerivation {
  pname = "foobar";
  inherit (src) version;

  inherit src;
}

Obviously we would have to implement version for all common fetchers, while some will need some extras such as tagPrefix.

In the case of fetchFromGitHub:

rev = if rev == null then
  "refs/tags/${toString tagPrefix}${version}"
else rev;

I think this could be a viable way forward, as it makes overriding sources much easier, while directly tying version to src.

[getchoo]

i believe we should be thinking of version with rev in the same way as we do pname and repo as described in #277994. this obviously isn't a one to one comparison (version is much more likely to be related to the rev after all), but i think it's important to not equate or tie this two together as they won't always be related. a big example here is with unstable- packages

as an alternative - and still keeping with the idea of using inputs - why not place version with src? like scrumplex said, those are the two main variables maintainers want to reuse. a pattern like so may work well

{
  stdenv,
  fetchFromGitHub,
  version ? "0.1.0",
  src ? fetchFromGitHub {
    owner = "foobar";
    repo = "foo";
    # here we can reuse `version` like before - or not! `src` and `version` are no longer tied
    # but both are still reusable, overridable, and accessible
    rev = "v${version}";
    hash = "...";
  }
}:
stdenv.mkDerivation {
  pname = "foo";
  inherit version src;
  
  # ...
  
}

i don't think this pattern is exactly new either, as i've seen (and used) it a good number of times in flakes to pass self.rev or similar

[AndersonTorres]

This should be a bit more polished.
As an example, I gathered all relevant sources for CBQN in one single file, sources.nix:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/bqn/cbqn/sources.nix

[oxij]

@getchoo

  version ? "0.1.0",
  src ? fetchFromGitHub {
    owner = "foobar";
    repo = "foo";
    # here we can reuse `version` like before - or not! `src` and `version` are no longer tied
    # but both are still reusable, overridable, and accessible
    rev = "v${version}";
    hash = "...";

But then, when you override the version, the hash will break it.

I agree with @Scrumplex's examples above, putting src into the inputs this way is superior. I would totally support an RFC wanting to allow doing that.

Also my #316668 seems very relevant here, given it introduces a bunch of lib.sources functions that can be useful here.

[Aleksanaa]

This isn't caused nor exacerbated by finalAttrs at all. If you override version and not src, even without finalAttrs, you'll also silently use the existing sources. That's just how FODs work.

The finalAttrs pattern in fact makes it easier to override src, as you may only need to set outputHash:

packageName.overrideAttrs (oldAttrs: {
  version = "0.1.1";

  src = oldAttrs.src.overrideAttrs {
    outputHash = "...";
  };
})

Without finalAttrs, you'd still need to set src, and more verbosely at that:

packageName.overrideAttrs (oldAttrs: {
  version = "0.1.1";

  src = fetchFromGitHub {
    repo = "...";
    owner = "...";
    rev = "...";
    hash = "...";
  };
})

Quoted from @eclairevoyant, original link #293452 (comment)

So it's still simpler to use finalAttrs.version.

[AndersonTorres]

I agree with @Scrumplex's examples above, putting src into the inputs this way is superior. I would totally support an RFC wanting to allow doing that.

Well I am doing some experiments, and indeed it is way superior:

#322093
#321056

[Aleksanaa]

Well I am doing some experiments, and indeed it is way superior:

#322093 #321056

Be careful. You don't know when the gnome people will come up with a project naming "sources".

[oxalica]

The finalAttrs pattern in fact makes it easier to override src, as you may only need to set outputHash:

packageName.overrideAttrs (oldAttrs: {
  version = "0.1.1";

  src = oldAttrs.src.overrideAttrs {
    outputHash = "...";
  };
})

Without finalAttrs, you'd still need to set src, and more verbosely at that:

packageName.overrideAttrs (oldAttrs: {
  version = "0.1.1";

  src = fetchFromGitHub {
    repo = "...";
    owner = "...";
    rev = "...";
    hash = "...";
  };
})

Quoted from @eclairevoyant, original link #293452 (comment)

So it's still simpler to use finalAttrs.version.

It almost got me¹, but yes, this do work as intended and can simplify the overriding code, as long as you already known this issue and check all usages of finalAttrs in that derivation.

However, the main point in the issue title still stands. It's introducing a trap when under-overriding hash causes a silent reuse of wrong FOD. Changing src without explicitly overriding it can be surprising for .overrideAttrs. I only expect .override to have this kind of complex dependent logic.

This isn't caused nor exacerbated by finalAttrs at all. If you override version and not src, even without finalAttrs, you'll also silently use the existing sources. That's just how FODs work.

As a overrideAttrs user for applying patches rather than upgrading/downgrading, it is exacerbated. By not using finalAttrs, overriding version overrides version, overriding src overrides src, and users can opt-in to override both if they want. That's literal semantics. By using finalAttrs.{pname,version}, overriding version automatically but partially override src, and now both users expecting an old src (want to patch) and users expecting a new src (want to upgrade/downgrade) are forced to write src = . If they missed it, building succeeds without error (even patches are correctly applied!) this time and breaks on the next GC.

Footnotes

1. I thought package.overrideAttrs (oldAttrs: { version = ".."; src = oldAttrs.src.overrideAttrs { outputHash = ".."; }; }) would use the new hash and the old version, because oldAttrs.src is the old attribute. Well, that's not the case. oldAttrs.src IS the old attribute, but its definition is recursively referencing finalAttrs.version, which correctly picks up the new overridden version. ↩

[eclairevoyant]

Then let's add version to the name of the FOD, then this shouldn't be an issue.
Or better, rev as in #294068

[Qyriad]

Well I am doing some experiments, and indeed it is way superior:

#322093 #321056

Be careful. You don't know when the gnome people will come up with a project naming "sources".

In all seriousness a problem of this nature is a genuine possibility, but prefixing simply named non-package arguments with an underscore, e.g. _src or _sources, would probably be a sufficient guard.

[Aleksanaa]

In all seriousness a problem of this nature is a genuine possibility, but prefixing simply named non-package arguments with an underscore, e.g. _src or _sources, would probably be a sufficient guard.

Yeah, we discussed in this issue #309892

[AndersonTorres]

Well I am doing some experiments, and indeed it is way superior:
#322093 #321056

Be careful. You don't know when the gnome people will come up with a project naming "sources".

Famous last words...

[Pandapip1]

This isn't caused nor exacerbated by finalAttrs at all. If you override version and not src, even without finalAttrs, you'll also silently use the existing sources. That's just how FODs work.
The finalAttrs pattern in fact makes it easier to override src, as you may only need to set outputHash:

packageName.overrideAttrs (oldAttrs: {
  version = "0.1.1";

  src = oldAttrs.src.overrideAttrs {
    outputHash = "...";
  };
})

Without finalAttrs, you'd still need to set src, and more verbosely at that:

packageName.overrideAttrs (oldAttrs: {
  version = "0.1.1";

  src = fetchFromGitHub {
    repo = "...";
    owner = "...";
    rev = "...";
    hash = "...";
  };
})

Quoted from @eclairevoyant, original link #293452 (comment)

So it's still simpler to use finalAttrs.version.

How does this work when using srcs as opposed to src?

[eclairevoyant]

Overriding lists doesn't work very well.