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

[RFC 0035] Default name from pname #35

Closed
wants to merge 7 commits into from

Conversation

Synthetica9
Copy link
Member

@Synthetica9 Synthetica9 commented Oct 2, 2018

Rendered

Currently looking for a co-author

@Synthetica9
Copy link
Member Author

cc @dtzWill @matthewbauer

@FRidh
Copy link
Member

FRidh commented Oct 2, 2018

Thanks for opening the RFC. It is indeed a very common and trivial pattern, so I am not against having that in our stdenv.mkDerivation. But, as I mentioned in the PR,

I'm of the opinion version should not actually be an attribute of the derivation but instead of the source.

@Synthetica9
Copy link
Member Author

Synthetica9 commented Oct 2, 2018

Thanks for opening the RFC. It is indeed a very common and trivial pattern, so I am not against having that in our stdenv.mkDerivation. But, as I mentioned in the PR,

I'm of the opinion version should not actually be an attribute of the derivation but instead of the source.

I disagree, different source versions might require different steps to build, install, etc. I feel it would be strange if this wouldn't be reflected in the derivation.

@FRidh
Copy link
Member

FRidh commented Oct 2, 2018

I disagree, different source versions might require different steps to build, install, etc. I feel it would be strange if this wouldn't be reflected in the derivation.

The version attribute does not consider that, but it is of course considered in the computation of the store path hash.

To be clear, I am not saying version should not be included in the derivation name, because I think it is good to have it there for convenience from a user point of view given the current tooling and when looking at store paths directly. However, it does not really make sense to have it as an attribute of the build artifact, because its an attribute of one of its dependencies: its source derivation. Same goes for license and homepage. The reason for including these in the derivation is again convenience/tooling support.

Anyway, what I state here fits I think better in the discussion/proposal of writing derivations differently (lost the link).

@Synthetica9
Copy link
Member Author

Synthetica9 commented Oct 2, 2018

Ah, sure, I can see that. However, this being such a simple RFC, I don't think it would do any good to drag this big a change into it.

@Ekleog
Copy link
Member

Ekleog commented Oct 3, 2018

Care should be taken […] to add an assertion that the generated name is consistent with the actual name if all three attributes are present (implemented in e0d2348).

Do the whole of nixpkgs still evaluate with this added assertion? I would fear that some packages may have eg. name = "${pname}-bis-${version} or similar stuff.

@Synthetica9
Copy link
Member Author

@Ekleog Good point! The original PR eventually switched to lib.strings.isSuffix instead of (==). This was due to the fact that Python packages defined their name as "${libversion}-${pname}-${version}", for example: "python2.7-setuptools-40.2.0" (which is currently the first example of such a failing package).

I see two options here:

  • Use the less common baseName instead of pname. I tested this, this seems to work out of the box. However, it would require some more refactors, and it is inconsistent with the currently official practice of using pname for Python packages.
  • Use hasSuffix. This is a less thorough consistency check, but seems to work as well.

P.S.

I couldn't get the entirety of nixpkgs to evaluate even unmodified, when I do nix eval "(import ./. {})" I get error: enum-0.4.4 not supported for interpreter python3.6m. Any pointers?

@Ekleog
Copy link
Member

Ekleog commented Oct 3, 2018

s/caviats/caveats/ on the latest commit :)

@Synthetica9 Synthetica9 changed the title Default name from pname [RFC 0035] Default name from pname Oct 3, 2018
@Profpatsch
Copy link
Member

Not really a fan, 700 places doesn’t look like a lot for a major source of implicitness (because yes, as you say, it’s going to be non-trivial to find out why the name is what it is, and where it is set.

@Synthetica9
Copy link
Member Author

Not really a fan, 700 places doesn’t look like a lot for a major source of implicitness (because yes, as you say, it’s going to be non-trivial to find out why the name is what it is, and where it is set.

Keep in mind, there is precedent: buildPythonPackage already does this.

@Ericson2314
Copy link
Member

Also, I eventually want pname and version to be separate meta fields, and Nix itself to name derivations. This is a good step in that direction.

@zimbatm
Copy link
Member

zimbatm commented Oct 3, 2018

pkgs.bundlerApp has a similar mechanism as well so it's not only python

@Synthetica9
Copy link
Member Author

pkgs.bundlerApp has a similar mechanism as well so it's not only python

Does this also only add a prefix to the standard pname-version scheme?

@timokau
Copy link
Member

timokau commented Oct 3, 2018

@zimbatm

Not really a fan, 700 places doesn’t look like a lot for a major source of implicitness (because yes, as you say, it’s going to be non-trivial to find out why the name is what it is, and where it is set.

I personally prefer that style and would have used it more often but was discouraged from doing so because it was not yet a standard. So the 700 may be a low number. If I could go back in time I would even use name instead of pname and give a longer name to whats currently called name since its generally (but not always) an implementation detail.

Also I think the consistency with python is very valuable.

@Synthetica9
Copy link
Member Author

@edolstra @zimbatm @shlevy Looks like all the discussion that is going to happen on this has happened, could I get a verdict?

@Profpatsch
Copy link
Member

From what I read in the comments, the general opinion is in favor of this change. I’d say “go forth and implement”.

@shlevy
Copy link
Member

shlevy commented Oct 29, 2018

Go for it IMO

@edolstra
Copy link
Member

This PR doesn't really explain why this change should be made. Presumably it's to allow tools like nix-env to unambigously distinguish between the name and version of a package, but it doesn't discuss what the impact on such tools is.

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 29, 2018

AFAIU it's just about simplifying the writing of a derivation, that's all.

@Synthetica9
Copy link
Member Author

This PR doesn't really explain why this change should be made. Presumably it's to allow tools like nix-env to unambigously distinguish between the name and version of a package, but it doesn't discuss what the impact on such tools is.

@Zimmi48 is correct, it is just a simplifying change to make it easier to write packages with less repeated "boilerplate".

@timokau
Copy link
Member

timokau commented Nov 1, 2018

I don't see any negatives here. What do you mean by churn?

@7c6f434c
Copy link
Member

7c6f434c commented Nov 2, 2018

The proposed change doesn't forbid direct name specification, it just reduces redundancy in case a better way (which is already also an idiom…) is used.

@dtzWill
Copy link
Member

dtzWill commented Nov 4, 2018

Don't suppose this could work for things like override or even overrideAttrs? O:)

@7c6f434c
Copy link
Member

7c6f434c commented Nov 5, 2018

I think working with override is very unlikely to be broken (it happens before the call to mkDerivation anyway), am I missing something?

@dtzWill
Copy link
Member

dtzWill commented Nov 5, 2018 via email

@7c6f434c
Copy link
Member

7c6f434c commented Nov 5, 2018 via email

@edolstra
Copy link
Member

Closing since this is already in Nixpkgs.

@edolstra edolstra closed this Jan 10, 2019
@Ericson2314
Copy link
Member

If something already exists matching the RFC (as opposed to conflicting), I rather see it merged. IMO this makes the outcome clearer at a glance, and is harmless because RFCs should specialize an idempotent course of action.

@edolstra
Copy link
Member

Merging it would imply that it was approved via the RFC process, which is not the case here.

@Ericson2314
Copy link
Member

Could the committee come to a decision retroactively? (keep vs revert)

@7c6f434c
Copy link
Member

@Ericson2314 I think the current status allows you some ways to promote pname usage without competing for the RFC processing bottleneck.

@dtzWill
Copy link
Member

dtzWill commented Jan 15, 2019 via email

@ghost ghost mentioned this pull request Jul 30, 2019
10 tasks
@dtzWill dtzWill mentioned this pull request Sep 5, 2019
10 tasks
@rkoe
Copy link

rkoe commented Sep 5, 2019

I'm not very happy with automagically adding variables in a non-obvious way.
It might save a few lines-of-code, but makes it less readable and so I doubt that it's worth it.
Explicit is better than implicit.

This RFC proposes that "name" should magically be added.
This would make package-definitons less readable/understandable, since then name exists with some value, but is not defined in the package-definition, and "comes out of nowhere".

In addition, pname isn't mentioned in the documentation at all; and this behaviour also isn't mentioned at all in the Nixpkgs-manual. So, if you want to go this way, PLEASE document it clearly in the Nixpkgs-manual, BEFORE promoting it.

@zimbatm
Copy link
Member

zimbatm commented Sep 5, 2019

I noticed that overriding packages has become more complicated. When pname + version is being used, overriding a package with overrideAttrs now requires to fix both the version and the name.

@Mic92
Copy link
Member

Mic92 commented Sep 6, 2019

I noticed that overriding packages has become more complicated. When pname + version is being used, overriding a package with overrideAttrs now requires to fix both the version and the name.

Can we automate this in overrideAttrs or is this too much magic?

@gloaming
Copy link

gloaming commented Sep 7, 2019

Where a package provides pname and version but no name, overrideAttrs already works fine in my testing.

The only problem I can see is when a package sets an explicit name using those, because the interpolation happens without a fixpoint. I presumed this was rare, but now I see that all python packages have it. It doesn't work for haskell packages either, even though they don't have a prefix.

Can we automate this in overrideAttrs or is this too much magic?

What automation do you propose? String substituting the old and new pname and version? It seems like a kludge, but it's a very nice kludge.

Alternatively, we could allow the name to be specified as a function { pname, version, ...}: ... but that seems like too much of a special case.

We could also require nixpkgs authors to fetch the pname and version from the fixpoint before using them in the name, but this doesn't help people writing packages outside of nixpkgs.

I guess the general solution would be for stdenv.mkDerivation to provide a fixpoint, and use with self instead of rec:

stdenv.mkDerivation (self: with self; {
  pname = "hello";
  version = "1.0";
  name = "prefix-${pname}-${version}";
}

Is there a reason this isn't done already? It does increase boilerplate.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/why-wouldnt-the-sha256-hash-change-when-fetching-from-a-different-url-when-updating-a-package/11745/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet