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

doc: update unstable version naming conventions #100833

Closed
wants to merge 1 commit into from

Conversation

ryantm
Copy link
Member

@ryantm ryantm commented Oct 17, 2020

The previous explanation did not make it clear where the word
"unstable" should go when name is split into pname and version.

By putting it at the end, we can get builtins.parseDrvName to parse
things nicely:

builtins.parseDrvName "pname-2020-10-17-unstable"

{ name = "pname"; version = "2020-10-17-unstable"; }

It is not good to put it in the pname, because then the pname will not
match with the attrpath.

Manual rendered:
image

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ryantm ryantm requested review from jtojnar and alyssais October 17, 2020 14:53
@ofborg ofborg bot added 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 17, 2020
@jtojnar
Copy link
Member

jtojnar commented Oct 17, 2020

As discussed on IRC, there are two orthogonal use cases: regular packages fetched from VCS (even temporarily), and projects that have both stable and unstable branch packaged. This PR currently covers the former (IMHO) correctly but we should probably also mention the latter.

cc @emilazy from #88023

@jtojnar
Copy link
Member

jtojnar commented Oct 17, 2020

Apparently, the nix-env behaviour is intentional, see its manual page:

Flags

  • --lt
    Only upgrade a derivation to newer versions. This is the default.

  • --leq
    In addition to upgrading to newer versions, also “upgrade” to derivations that have the same version. Version are not a unique identification of a derivation, so there may be many derivations that have the same version. This flag may be useful to force “synchronisation” between the installed and available derivations.

  • --eq
    Only “upgrade” to derivations that have the same version. This may not seem very useful, but it actually is, e.g., when there is a new release of Nixpkgs and you want to replace installed applications with the same versions built against newer dependencies (to reduce the number of dependencies floating around on your system).

  • --always
    In addition to upgrading to newer versions, also “upgrade” to derivations that have the same or a lower version. I.e., derivations may actually be downgraded depending on what is available in the active Nix expression.

For the other flags, see --install.

Maybe we could make --always default, though.

@AluisioASG
Copy link
Contributor

A bit of bikeshedding, but here's a rewording I find easier to follow:

If a package is not a release but a commit from a repository, then the version part of the name must be the date of that (fetched) commit in "YYYY-MM-DD" format, followed by "-unstable" — e.g., "pkgname-2014-09-23-unstable".

@ryantm
Copy link
Member Author

ryantm commented Oct 19, 2020

Incorporated @AluisioASG 's suggestion.
Rendered:
image

@jtojnar
Copy link
Member

jtojnar commented Oct 19, 2020

Could we also mention that “If there are multiple version branches of a project installed, their pnames need to be distinct. For example fooUnstable should have "foo-unstable" as pname and fooDev should have "foo-dev" as pname. This can lead to having -unstable suffix both in pname and version but it should not matter.”

@ryantm ryantm changed the title doc: update unstable-version package naming conventions doc: update version and pname package naming conventions Oct 20, 2020
@ryantm
Copy link
Member Author

ryantm commented Oct 20, 2020

Updated to include @jtojnar's suggestion.

Rendered:

image

@fgaz
Copy link
Member

fgaz commented Oct 20, 2020

I agree that "unstable" should not be in the pname.

More bikeshedding: I liked it better at the beginning of the version because that's the most significant part, and it makes sense to first differentiate between stable and unstable, and then by the rest of the version.
On the other hand, "unstable" is really just a tag/attribute, not part of the version number itself, so it's fine at the end too.
edit: if parseDrvName will not be changed, I agree it's best to keep unstable at the end, so pname, version, and parseDrvName match.

Overall strong +1!

@gebner
Copy link
Member

gebner commented Oct 20, 2020

Could we also mention that “If there are multiple version branches of a project installed, their pnames need to be distinct. For example fooUnstable should have "foo-unstable" as pname and fooDev should have "foo-dev" as pname. This can lead to having -unstable suffix both in pname and version but it should not matter.”

I think we should only require foo-unstable-YYYY-MM-DD if both versions can be installed at the same time (i.e., do not have conflicting files). For example, nixUnstable and nixStable should both have nix as pname, because they both provide the nix executable.

@jtojnar
Copy link
Member

jtojnar commented Oct 20, 2020

I think we should only require foo-unstable-YYYY-MM-DD if both versions can be installed at the same time (i.e., do not have conflicting files). For example, nixUnstable and nixStable should both have nix as pname, because they both provide the nix executable.

We need to distinguish them because otherwise, nix-env might upgrade to the unstable package. At least that it what I've heard. But I see we do not distinguish them for nixStable and nixUnstable so 🤷‍♀️

@gebner
Copy link
Member

gebner commented Oct 20, 2020

We need to distinguish them because otherwise, nix-env might upgrade to the unstable package.

At least for me, that's the whole point. If I run nix-env -f'<nixpkgs>' -iA nixUnstable, then I want the unstable version of nix, not the stable one.

(I know about nix-env -u, of course. But I typically use nix-env to install custom patched versions, e.g. where the PR hasn't hit the channels yet. Using nix-env -u would completely defeat this purpose.)

@raboof
Copy link
Member

raboof commented Oct 20, 2020

As discussed on IRC, there are two orthogonal use cases: regular packages fetched from VCS (even temporarily), and projects that have both stable and unstable branch packaged

For the VCS use case: perhaps we should also consider whether unstable is the right marker at all? Often that version is not particularly 'unstable', just not assigned an explicit version number. Perhaps something else, like -snapshot, -git, -vcs, ... would make more sense?

For the 'projects that have both stable and unstable branch packaged' case: how does this relate to packages like etcd, where there's a etcd and a etcd_3_4? Should etcd_3_4 also be the pname? What about packages that are known under multiple names (e.g. jdk also being jdk14)?

@ryantm
Copy link
Member Author

ryantm commented Oct 23, 2020

I feel like this is getting bogged down on the pname naming part. Maybe we can just merge the version part and leave the pname part for later, @jtojnar?

The previous explanation did not make it clear where the word
"unstable" should go when name is split into pname and version.

By putting it at the end, we can get builtins.parseDrvName to parse
things nicely:

> builtins.parseDrvName "pname-2020-10-17-unstable"

{ name = "pname"; version = "2020-10-17-unstable"; }

It is not good to put it in the pname, because then the pname will not
match with the attrpath.
@ryantm ryantm changed the title doc: update version and pname package naming conventions doc: update unstable version naming conventions Oct 23, 2020
@ryantm
Copy link
Member Author

ryantm commented Oct 23, 2020

I updated this to remove the pname part for now, since it needs more thought.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to also allow other postfixes rather than only -unstable (and perhaps even other patterns, such as git describe-style, could also be useful).

That said, this is already seems like an improvement over the previous convention AFAICS, so 👍

@jtojnar
Copy link
Member

jtojnar commented Oct 23, 2020

I would like to hear from @edolstra first, as he was vaguely opposed to this IIRC.

@lopsided98
Copy link
Contributor

lopsided98 commented Nov 16, 2020

Note that while the official status quo is to suffix pname, the reality is somewhat different:

  • pname = "<name>-unstable": 121 matches
  • version = "unstable-YYYY-MM-DD": 346 matches
  • version = "YYYY-MM-DD-unstable": 3 matches

We may want to write a script to automatically convert unstable version prefixes to suffixes. This would help people use the right format with looking at other packages for examples.

@zimbatm
Copy link
Member

zimbatm commented Nov 17, 2020

A related discussion to have is how to map attribute names to pnames. Having that would allow looking at the store path or nix-env -q output and find back the right attribute.

In that regard, I think we should reserve _ (underscore) to postfix the pname with variations. For example nix_unstable, or terraform_0_11 would be both the attribute and pname.

For packages that live in a package set like haskellPackages, we can argue whenever those pnames should also include haskellPackages. as a prefix to the pname, or not.

@edolstra
Copy link
Member

In that regard, I think we should reserve _ (underscore) to postfix the pname with variations.

nix-env -qa | grep _ suggests that there are too many packages with an underscore in the name to make this feasible.

And in any case, it would repeat the mistake of having special characters in the name to denote some meaning (i.e. using - followed by a digit to denote the version). It would be cleaner to have a variant attribute in addition to pname and version.

@ryantm
Copy link
Member Author

ryantm commented Nov 18, 2020

@zimbatm @edolstra what's your opinion about this PR?

If we can't come to consensus about where "unstable" should go, we should probably remove the official guidance about using it.

@edolstra
Copy link
Member

unstable (or other variants like minimal or full) should come before the version. That way nix-env sees it as a separate package and won't try to upgrade to it. E.g. firefox-unstable-84.0 is a different package from firefox-83.0, whereas firefox-84.0-unstable is a more recent version of the same package as firefox-83.0.

@jtojnar
Copy link
Member

jtojnar commented Nov 18, 2020

Most of the time there is only unstable package. Either the stable package is so out of date we decided to pull it from git or there were no releases in the first place but there might be some in the future. In those cases, we want nix-env to treat them as the same package. That is why I proposed having two -unstable suffixes – one for pname and another for version.

@emilazy
Copy link
Member

emilazy commented Nov 18, 2020

NixOS/nix#3579 (comment) and #88023 (comment) seem relevant here.

@AluisioASG
Copy link
Contributor

I think this discussion would benefit from @raboof 's suggestion to distinguish "fetched from VCS"-unstable from "unstable package branch"-unstable.

@zimbatm
Copy link
Member

zimbatm commented Nov 22, 2020

It's a multi-dimensional problem so it's hard to find a good solution.

I still think that the pname should match the attribute name as much as possible for practical reasons. Eelco is right that _ cannot have a special meaning but this doesn't invalidate the basic premise. @jtojnar also made a good point that sometimes there Is only one package, that is unstable. I would even go further and say that this particular package can oscillate between stable and unstable over time. Each time causing nix-env -u upgrade problems. nix profile is using attribute names so that should also be taken into account, and adds weight to the attr name <-> pname mapping.

@raboof raboof mentioned this pull request Jan 9, 2021
10 tasks
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 18, 2021

Bump.

This needs clearing up because right now we do pname = "name"; version = "unstable-XXXX-XX-XX";.

@zimbatm
Copy link
Member

zimbatm commented Jan 18, 2021

The original argument was to say that the output of builtins.parseDrvName should match the pname and version attributes. I feel like this hasn't really been discussed.

nix-repl> pname = "foo" 

nix-repl> version = "unstable-2020-10-01"

nix-repl> builtins.parseDrvName "${pname}-${version}"                      
{ name = "foo-unstable"; version = "2020-10-01"; }

This would dictate that -unstable should go after the date. Or that it should be part of the pname attribute instead of the version (which respects Eelco's wish).

Similarly, if we want to have multiple variants of a package, then those would be encoded in the pname.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 18, 2021

builtins.parseDrvName

I don't understand why we can't change that builtin to match what we want.

This would dictate that -unstable should go after the date. Or that it should be part of the pname attribute instead of the version (which respects Eelco's wish).

This means it would need a treewide change after which the date in the version confuses repology even more than right now.

Similarly, if we want to have multiple variants of a package, then those would be encoded in the pname.

The only packages we have multiple of are big compilers, interpreters and some others but they almost always have a version number in their pname because we are almost never tracking git commits on them.

The most common use case of not tracking a release version right now is when something broke with some dependency and the patch does not apply on the latest release because it is simply to old or the latest release is just simply to old. Such packages have almost never two versions in nixpkgs and the unstable is almost always with the version number infront of it.

@jtojnar
Copy link
Member

jtojnar commented Jan 19, 2021

The original argument was to say that the output of builtins.parseDrvName should match the pname and version attributes.

That function is terrible by design and cannot really be fixed (compare darcs-to-git-2015-06-04 and twmn-git-2018-10-01) so I would argue that it should be deprecated. Instead, we should use lib.getName and lib.getVersion, which are aware of pname and version attributes.

I have also made a PR with similar change against nix-env -q so that Repology can work.

@edolstra
Copy link
Member

I don't understand why we can't change that builtin to match what we want.

Compatibility.

That function is terrible by design and cannot really be fixed

There is nothing broken about that function. It exposes exactly the semantics that nix-env uses to split names and versions. So starting a version with a letter is by definition wrong.

@jtojnar
Copy link
Member

jtojnar commented Jan 19, 2021

There is nothing broken about that function. It exposes exactly the semantics that nix-env uses to split names and versions. So starting a version with a letter is by definition wrong.

It is broken by design because it follows broken semantics of nix-env. The semantics is broken because it does not allow unambiguous encoding of versions starting with letters and project names containing a dash followed by a digit.

In other words, sure, it follows specification but the specification itself is misdesigned.

@davidak
Copy link
Member

davidak commented Jan 22, 2021

I like the idea to have variant in addition to pname and version.

And in case of a package that has no stable version or the stable version is too old, use variant = "snapshot";. It is not necessary "unstable".

When variant is not present, a package is considered stable. variant = "unstable"; or variant = "full"; can be used to separate a package from it's default stable version. We should standardize these options and document them! Then repology can just ignore them and group our package with the other versions.

Some current edge cases:

  • pname = "ghdl-${backend}"; (where backend can be "mcode" or "llvm") -> repology recognizes both as packages, so it's OK
  • name = "asus-wmi-sensors-${version}-${kernel.version}"; -> variant could be linux-${kernel.version}
  • name = "gitlab${lib.optionalString gitlabEnterprise "-ee"}-${version}"; -> can be variant
  • name = "VirtualBox-GuestAdditions-${version}-${kernel.version}" -> variant could be linux-${kernel.version}
  • name = "gimp-plugin-${name}"; -> i guess that's ok. make it pname. Fedora uses gimp-${name}-plugin
  • name = "${pname}${if withGnome then "-gnome" else ""}-${version}"; -> seem fine

Do you see any case where variant would not work?

The string that represents a package in nix-env could be considered a display name that is only meant for a user to read, not used in any technical way. It can be name = "${pname}-${variant}-${version}"; while in case variant is empty, also the - is removed. Nix should use proper metadata when available and should differentiate between pname and version. This should only be an implementation detail that can be changed at any time without breaking anything, like the presentation of the version in the about dialog of a GUI software.

To be compatible with previous nixpkgs versions (that is a feature after all), we can have additional functions to handle it. That is an additional burden, but it's the price for compatibility when we want to move forward at the same time.

@edolstra what do you think about the idea to keep the old name handling code around in Nix to handle old nixpkgs versions? would that solve any compatibility problems you see?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/brainstorming-for-rfc-pname-and-version/12873/1

@ryantm ryantm removed this from the 21.05 milestone May 4, 2021
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/brainstorming-for-rfc-pname-and-version/12873/2

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/brainstorming-for-rfc-pname-and-version/12873/58

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 3, 2021
@ryantm
Copy link
Member Author

ryantm commented Jun 11, 2021

Closing this since it looks like people are working on an RFC instead.

@ryantm ryantm closed this Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.