Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mkPythonDerivation: let name default to ${pname}-${version} #31173

Merged
merged 1 commit into from Nov 3, 2017

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Nov 3, 2017

Motivation for this change

Python packages that use fetchPypi typically refer only to their pname and version instead of name, but have to define name = "${pname}-${version}"; to conform to the interface of mkDerivation. @FRidh has proposed to get rid of this.

This commit rebuilds nothing. I've tested it by deleting the name from a python derivation. If we migrate python-packages.nix into python-modules/ automatically, we could delete unneeded names in the process.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@FRidh
Copy link
Member

FRidh commented Nov 3, 2017

pname and version are not (yet) mandatory parameters. That allows you to have an almost empty derivation name. I'm not sure what's preferable at this moment, not having to write name = "${pname}-${version};, or risking not having a name. I'll guess it will be rare for someone to forget passing in a (p)name.


Some more clarification. I very much like this change and want this to happen as well, but I think we need to be sure whether this is the right direction. That is, perhaps we should fill in pname and version as much as possible. When most Python derivations have those attributes, we could then make this change, but at the same time also make pname and version mandatory. What do you think?

@orivej
Copy link
Contributor Author

orivej commented Nov 3, 2017

Specifying neither name nor pname and version results in the evaluation error. Please try it to see if the error is clear enough.

@orivej
Copy link
Contributor Author

orivej commented Nov 3, 2017

By the way, do you want to replace the usage of fetchurl with fetchPypi? If so, why?

@FRidh
Copy link
Member

FRidh commented Nov 3, 2017

By the way, do you want to replace the usage of fetchurl with fetchPypi? If so, why?

To easily fetch another version. See e.g. a3e5240#diff-9a467ecbd5ed7def480033cf9289509aR9

@FRidh
Copy link
Member

FRidh commented Nov 3, 2017

Tested it. Great!

@FRidh FRidh merged commit 844ef8c into NixOS:master Nov 3, 2017
@FRidh FRidh mentioned this pull request Nov 7, 2017
8 tasks
@orivej orivej deleted the python-implicit-name branch November 13, 2017 00:39
@FRidh
Copy link
Member

FRidh commented Nov 23, 2017

Unfortunately it doesn't work as I hoped it would.

Building

foo.overrideAttrs (oldAttrs: {
  version = "1.2";
})

will give a version of foo that still has the old version in its name.

@orivej
Copy link
Contributor Author

orivej commented Nov 23, 2017

@FRidh You have to use overridePythonAttrs.

@FRidh
Copy link
Member

FRidh commented Nov 23, 2017

Ohh doh, how did I miss that. Thanks!

FRidh added a commit that referenced this pull request Jan 16, 2018
The `name` attribute is filled in when `pname` and `version` are specified.
See #31173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants