-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Superfluous use of finalAttrs.{pname,version} introducing overriding traps #310373
Description
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=codefinalAttrs.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.