[WIP/RFC] make-derivation: default name if pname and version are both set#41627
[WIP/RFC] make-derivation: default name if pname and version are both set#41627Synthetica9 wants to merge 20 commits intoNixOS:stagingfrom
Conversation
|
I'm not sure how important it is but I think it's common for these to be |
matthewbauer
left a comment
There was a problem hiding this comment.
Definitely looks like something very useful! pname/version is what lots of packages are already using so hopefully everyone is okay with this.
| # Explanation about derivations in general | ||
| mkDerivation = | ||
| { name ? "" | ||
| { name ? if builtins.hasAttr "pname" attrs && builtins.hasAttr "version" attrs |
There was a problem hiding this comment.
Can you update "pos" above as well? It is used to find line numbers in traces. I think we need to also look for "version" before name for this to work correctly.
There was a problem hiding this comment.
Would that be something like builtins.unsafeGetAttrPos "pname" attrs?
There was a problem hiding this comment.
Yeah but I actually think version would be better because it's more frequently used than pname.
There was a problem hiding this comment.
I'd like to have an assertion for name == "${pname}-${version}" if all of the three are set
There was a problem hiding this comment.
Oh, yeah, that would make sense. Adding that.
There was a problem hiding this comment.
This should be attrs ? pname && attrs ? version.
|
@globin I suspect we should document this as well? |
|
Try removing pname & version from attrs in python here: |
|
@matthewbauer Looks like a good first step, but crashes here (and someone foresaw it :P): |
|
Yeah we might be able to just remove the prefix there as well. /cc @FRidh |
|
I'm of the opinion version should not actually be an attribute of the derivation but instead of the source. |
That's a good point - but the way Nixpkgs works now is that version is an attribute of the derivation through name - this just lets you separate those two out. |
We include it in the name, but it is not an attribute. Therefore, it is possible to have |
|
I've pushed b5d7bb4 (removing name from Python packages) to staging. |
In the case of Python, you could set and remove the |
|
The following should also work I think: let
pname = "setuptools";
version = "39.0.1";
in stdenv.mkDerivation rec {
name = "${python.libPrefix}-${pname}-${version}"; |
@matthewbauer Actually, that also seems to break things: |
952049c to
2b1c0fc
Compare
2b1c0fc to
b03d631
Compare
too greedy find-replace
|
I think I'm starting to come to the conclusion that dropping pname and/or version from pythonpackages is just a bad idea (people find creative ways to use both), and we should just change the assert to |
You are referring to the following code: psutil_1 = self.psutil.overrideAttrs (oldAttrs: rec {
name = "${oldAttrs.pname}-${version}";
version = "1.2.1";
src = oldAttrs.src.override {
inherit version;
sha256 = "0ibclqy6a4qmkjhlk3g8jhpvnk0v9aywknc61xm3hfi5r124m3jh";
};
});Here they need to set With your proposed change, If you like to I can have a look at how |
|
A fundamental change like this is something that should be done via the RFC process, IMHO. |
|
Failure on x86_64-linux (full log) Attempted: ibus Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: ibus Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: ibus Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: ibus Partial log (click to expand)
|
|
Superseded by NixOS/rfcs#35 |
Motivation for this change
This is done in, for example,
buildPythonPackageTODO
name = "${pname}-${version}"from packages that define it that way (ef6ece0)${name}in thefetchUrlanymore (seeaf1b4f4)
name = "foobar-${version}"for topname = "foobar"for packages that define it that way (WIP on my machine, will give a lot of pings once I push it: there are 3k+ packages that use this method.)recfrom packages that don't need it anymore