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

treewide: split name into pname/version for unstable packages #88023

Closed
wants to merge 1 commit into from

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented May 17, 2020

Motivation for this change

A little step forwards to untangle the unstable versioning mess.

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.

@pbogdan
Copy link
Member

pbogdan commented May 17, 2020

This may not be the best place to ask but something I've wondered for a while - why does the unstable part belong to the version component rather than pname? https://nixos.org/nixpkgs/manual/#sec-package-naming explicitly says it should be part of the package name and not the version, and as far as Nix is concerned (parseDrvName) everything up to first digit preceded by a - is considered the name of the package i.e. foo-unstable-01-01-1970 parses into foo-unstable name and 01-01-1970 version which is not consistent with how many packages specify the individual parts.

@eadwu
Copy link
Member

eadwu commented May 17, 2020

Actually I think there was a consensus (maybe not?) before that the unstable part should just be removed while the date remains the version, if I remember correctly.

@prusnak
Copy link
Member Author

prusnak commented May 17, 2020

explicitly says it should be part of the package name and not the version

That's not what the documentation says, it only says the name should be pkgname-unstable-2014-09-23 - it does not say where the unstable comes from, whether from pname or version.

And AFAIK there is no consensus yet (as confirmed by @jtojnar on IRC).

$ rg 'pname.*unstable' | wc -l
112
$ rg 'version.*unstable' | wc -l
316

Currently, there are at least 3 options being discussed:

a) pname = "foo-unstable"; version = "YYYY-MM-DD";
b) pname = "foo"; version = "unstable-YYYY-MM-DD";
c) pname = "foo"; version = "YYYY-MM-DD-unstable";
... + some others (such as introducing unstable = true; or unofficial = true; flags)

My intention with this change is to untangle name into pname and version in unstable packages and make the transition to whichever option easier.

If there is a strong opposition against b) option in this PR, I will rework it to be in a) format. I am fine with both options, honestly.

@Mic92
Copy link
Member

Mic92 commented May 17, 2020

I vote for option a) pname = "foo-unstable"; version = "YYYY-MM-DD"; to be consistent with parseDrvName

@prusnak
Copy link
Member Author

prusnak commented May 17, 2020

Force pushed and used option a) instead of option b) as per comment above.

@emilazy
Copy link
Member

emilazy commented May 17, 2020

I dislike including -unstable in the pname because it means changes that only touch the version of the software will change its pname, which seems to rather defeat the point of separating them out.

My preferred option would be as @eadwu said, just dropping the -unstable and using version = "YYYY-MM-DD", but if I recall correctly nix-env(1) relies on it somehow?

@kini
Copy link
Member

kini commented May 17, 2020

That's not what the documentation says, it only says the name should be pkgname-unstable-2014-09-23 - it does not say where the unstable comes from, whether from pname or version.

Well, it's kind of unclear. First it says:

In Nixpkgs, there are generally three different names associated with a package:

  • The name attribute of the derivation (excluding the version part). This is what most users see, in particular when using nix-env.
  • The variable name used for the instantiated package in all-packages.nix, and when passing it as a dependency to other functions. Typically this is called the package attribute name. This is what Nix expression authors see. It can also be used when installing using nix-env -iA.
  • The filename for (the directory containing) the Nix expression.

So it seems reasonable to assume that when it later refers to "the name", the "(excluding the version part)" is implied, i.e. it's referring to what we'd now call pname. Furthermore, there is this sentence:

Also append "unstable" to the name - e.g., "pkgname-unstable-2014-09-23".

If "the name" was meant to refer to the entire name rather than pname, then appending "unstable" to "the name" would result in "pkgname-2014-09-23-unstable". If "the name" was meant to refer to the pname, then it would result in "pkg-name-unstable-2014-09-23" which matches the example given. So I think it's safe to say that "the name" is intended to refer to pname.

However I agree with @eadwu and @emilazy that ideally the unstable-ness could be indicated in some other way, so that the pname doesn't change when nixpkgs switches between a stable released version and an intermediate untagged commit.

Another reason not to use option b) is that the docs specifically say:

The version part of the name attribute must start with a digit (following a dash) — e.g., "hello-0.3.1rc2".

@Mic92
Copy link
Member

Mic92 commented May 18, 2020

I dislike including -unstable in the pname because it means changes that only touch the version of the software will change its pname, which seems to rather defeat the point of separating them out.

My preferred option would be as @eadwu said, just dropping the -unstable and using version = "YYYY-MM-DD", but if I recall correctly nix-env(1) relies on it somehow?

I think the reason for having it in the first place is to differentiate between unstable and stable packages when using nix-env -i for imperative package management. I am not a fan of nix-env but it seems this feature is also not going away with newer versions of the nix cli so we need to keep the -unstable- part to not confuse nix-env when upgrading.

@Mic92
Copy link
Member

Mic92 commented May 18, 2020

However I agree with @eadwu and @emilazy that ideally the unstable-ness could be indicated in some other way, so that the pname doesn't change when nixpkgs switches between a stable released version and an intermediate untagged commit.

That would be great however also a quite invasive break that also needs support in nix-env, so I would not do this right now.

@Mic92
Copy link
Member

Mic92 commented May 18, 2020

I vote for option a) pname = "foo-unstable"; version = "YYYY-MM-DD"; to be consistent with parseDrvName

Beeing consistent with nix builtins also means that tools/services that evaluate nix will show versions the same way the user sees it in the code, i.e. https://repology.org, nixpkgs-review or https://lazamar.co.uk/nix-versions/

@prusnak
Copy link
Member Author

prusnak commented May 18, 2020

It seems there are good reasons to stick with a) for now until a better mechanism is invented. Once this is merged, I'll send a followup PR which moves the unstable part from version to pname in the remaining 316 packages.

@jtojnar
Copy link
Member

jtojnar commented May 18, 2020

The issue with having it in pname is that there is no longer a nice way to get the project name. You can only try stripping -unstable and hope the project name did not contain it.

To repeat myself from #86430 (comment), I think we should stop using parseDrvName. It sort of works for nix-env but not for anything else.

@kini
Copy link
Member

kini commented May 18, 2020

There are also a fair number of packages which use name and have a version that is a date but don't have "unstable" in name at all (some of these are false positives, though):

$ rg 'name *=.*[0-9]{4}-?[0-9]{2}-?[0-9]{2}"' | grep -v unstable | wc -l
184

These should probably be cleaned up at some point too.

There may also be some packages which use pname and version and are missing the "unstable", too, but a simple grep wouldn't suffice to find them all, I guess, since pname and version don't need to be specified in textually adjacent locations or even in the same file in general.

@Mic92
Copy link
Member

Mic92 commented May 18, 2020

The problem arises if upstream uses the same format to specify versions and we have no unstable identifier.

@Mic92
Copy link
Member

Mic92 commented May 18, 2020

To repeat myself from #86430 (comment), I think we should stop using parseDrvName. It sort of works for nix-env but not for anything else.

What is the alternative?

@jtojnar
Copy link
Member

jtojnar commented May 18, 2020

What is the alternative?

Split the name into pname in every derivation packaging a software project and use pkg.pname and pkg.version, and consider derivations without a pname not a software package.

@emilazy
Copy link
Member

emilazy commented May 18, 2020

I am not a fan of nix-env but it seems this feature is also not going away with newer versions of the nix cli so we need to keep the -unstable- part to not confuse nix-env when upgrading.

I was kind of hoping the new nix profile stuff would at least have less of this kind of magic and fragility of nix-env(1), but I haven't played with it yet... I guess it still uses the same logic for versioning/upgrades?

To me, including -unstable in the pname seems to defeat the point of having a pname at all. I'd prefer the version to actually be the version and treat parseDrvName as the thing with the issue. But the inconsistency with the rest of the ecosystem is unfortunate...

@emilazy
Copy link
Member

emilazy commented May 18, 2020

Come to think of it, is the behaviour where nix-env won't upgrade to unstable versions because it sees them as different package names due to the -unstable actually desirable? It means that someone upgrading to a NixOS version that moved a package from an outdated stable release to a git commit will remain stuck on the old package indefinitely, won't it?

It seems to me that usually if a package has moved to an unstable version it's for good reason, and we'd want users to be running that version. There's a distinction between "unstable package you don't want to install yet" and "package that uses an unstable revision as a base", and it seems like the former belongs more in the domain of "what nixpkgs branch you run" rather than "what version of a package you have installed".

To be clear, this is an argument for removing the -unstable from the package name. I think I would now prefer (c) of the options listed, as version = "YYYY-MM-DD-unstable" should both parse correctly by parseDrvName/nix-env, and explicitly mark unstable versions as distinct from upstream releases that happen to use that versioning format.

@Mic92
Copy link
Member

Mic92 commented May 19, 2020

It seems to me that usually if a package has moved to an unstable version it's for good reason, and we'd want users to be running that version. There's a distinction between "unstable package you don't want to install yet" and "package that uses an unstable revision as a base", and it seems like the former belongs more in the domain of "what nixpkgs branch you run" rather than "what version of a package you have installed".

The problem is however that we have both unstable/stable packages for some packages. In that case nix-env would most often prefer the unstable version over the stable version i.e. 1.0.0 < 2020-05-01-unstable

@jtojnar jtojnar mentioned this pull request Jun 10, 2020
10 tasks
@jtojnar jtojnar mentioned this pull request Jun 24, 2020
10 tasks
@jtojnar jtojnar mentioned this pull request Jul 5, 2020
10 tasks
@romildo romildo mentioned this pull request Jul 9, 2020
10 tasks
@prusnak
Copy link
Member Author

prusnak commented Jul 20, 2020

I am closing this. When there is a consensus how to do this, we can revisit.

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.

8 participants