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

Projects
None yet
@Synthetica9
Copy link

commented Oct 2, 2018

Rendered

Currently looking for a co-author

Synthetica9 added some commits Oct 2, 2018

@Synthetica9

This comment has been minimized.

Copy link
Author

commented Oct 2, 2018

@FRidh

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Oct 3, 2018

@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?

Synthetica9 added some commits Oct 3, 2018

@Ekleog

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

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

This comment has been minimized.

Copy link
Author

commented Oct 3, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

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

@Synthetica9

This comment has been minimized.

Copy link
Author

commented Oct 3, 2018

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

This comment has been minimized.

Copy link
Member

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.

@jtojnar jtojnar referenced this pull request Oct 26, 2018

Merged

Package modem-manager-gui #48959

3 of 9 tasks complete
@Synthetica9

This comment has been minimized.

Copy link
Author

commented Oct 28, 2018

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

@Profpatsch

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

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

@shlevy

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Go for it IMO

@edolstra

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

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

This comment has been minimized.

Copy link

commented Oct 29, 2018

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

@Synthetica9

This comment has been minimized.

Copy link
Author

commented Oct 29, 2018

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".

@Synthetica9 Synthetica9 referenced this pull request Oct 29, 2018

Merged

Implement rfc0035: default `name` from `pname` #49398

2 of 9 tasks complete
@Synthetica9

This comment has been minimized.

Copy link
Author

commented Oct 29, 2018

Go for it IMO

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

@shlevy @Profpatsch NixOS/nixpkgs#49398

@dezgeg

This comment has been minimized.

Copy link

commented Oct 29, 2018

There are/would be other advantages besides simplifying expressions for derivations:

  1. This approach won't break if a package's name contains a dash followed by a number. E.g. you currently can't have the name be something like ntfs-3g-${version} because that will be misinterpreted .
  2. Often there's a need to attach some extra suffix to a package's name. For example the tp_smapi package (which is a kernel module) has:
    name = "tp_smapi-${version}-${kernel.version}";
    version = "0.43";
    
    and it's sometimes convenient to be able to know just the version and not the extra suffix.

It's worth noting that lib.getVersion from Nixpkgs already uses the version attribute if that's specified and only then falls back to builtins.parseDrvName

@timokau

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

After adding a new kernel module I just noticed that repology will be confused by the naming scheme @dezgeg mentions. It will list the kernel module version as the kernel version. The current PR wouldn't allow setting pname, version and name. Should we maybe change the kernel module naming scheme instead?

@Synthetica9

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

@timokau I'd say it doesn't fit the Nixpkgs manual Package naming.

Maybe kernel${kernel.version}-${pname}-${version}?

@timokau

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Thats true, but it is how it is commonly done. Not sure who came up with it first and why. I'd support a naming scheme change.

@matthewbauer

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

I agree with @FRidh who said that version really shouldn't be a part of the derivation at all. It really should be part of "src".

I don't doubt that there are advantages to splitting name and version up, but I'm not sure if it outweighs the cost of churn here. The name = "hello-1.0 idiom has been there for over 10 years (https://nixos.org/~eelco/pubs/nspfssd-lisa2004-final.pdf). I don't think there's a compelling enough reason to change it yet.

@timokau

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

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

@7c6f434c

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2018

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

@7c6f434c

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

@edolstra

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Closing since this is already in Nixpkgs.

@edolstra edolstra closed this Jan 10, 2019

@Ericson2314

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

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

@Ericson2314

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

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

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

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

@dtzWill

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.