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

nix profile: Allow referring to elements by human-readable name #8678

Merged
merged 6 commits into from
Dec 21, 2023

Conversation

iFreilicht
Copy link
Contributor

@iFreilicht iFreilicht commented Jul 9, 2023

Motivation

This PR replaces the index as the main handle for user-interaction with packages with a heuristically determined, human-readable name.

This enables commands nix profile upgrade and nix profile remove to refer to packages by their expected name like so:

$ nix profile install nixpkgs#vim
$ nix profile upgrade vim
$ nix profile remove vim

This is desirable as the index is a surprising interaction handle and the better attribute path requires matching with regex to use properly, otherwise the user gets a surprising error message. Using a name that closely matches what the user would expect is more intuitive and in line with the interface of other CLI package managers.

Context

This PR is a starting point for improving the UX of nix profile in general, see #7966.
In particular, it implements the feature requested in #7967 to fix #7960, #7961 and #7962.

The old index is still displayed, but using it to upgrade or remove a package prints a deprecation warning. Actually removing the index will be done in #9171.

Implementation strategy

A heuristic approach is used that generates the names when the profile manifest is loaded. The names are not written to the manifest, and so its structure does not change, making this change 100% backward- and forward-compatible. I deem this a good approach as it allows easy additions to these heuristics should we run into edge-cases in the future.

Duplicate packages are handled by suffixing the name with a number. This is simple and effective, and I assume we don't hit that case too often in reality.

Manually overriding the auto-generated name is not supported yet, as this would only work when installing a single package at once with the current evaluation of command line arguments, which I deemed out-of-scope for this PR.

This change is compatible with profiles managed by nix-env as well as profiles managed by nix profile that still have old packages installed by nix-env in them.

To avoid confusing "does not match any packages" warnings, this PR also adds additional warning messages when packages are found, but can't be upgraded due to having been installed via nix-env, a store-path or a locked flake ref:

warning: Found package 'vim', but it was not installed from a flake, so it can't be checked for upgrades!
warning: Found package 'github:NixOS/nixpkgs/842e90934a352f517d23963df3ec0474612e483c#legacyPackages.x86_64-linux.vim', but it was installed from a locked flake reference so it can't be upgraded!

this does not fix silently skipping upgrades for pinned flakes, though, see #8704.

Alternatives

I also considered storing the generated names in the manifest.json but decided against it to ensure potential edge-cases in the heuristics would not be permanently stored in the profile. This will us to change the heuristics quickly without any concerns about incompatibility and users will immediately benefit from upgrades without us having to implement additional mechanisms for overriding names manually, upgrading the generated names or anything like that.

We could add the names to the output of nix profile list --json, and that might actually be useful, but I decided against it for now as I'm not sure we want that command's output to differ from manifest.json.

For deduplication of names, we might want a smarter strategy or for the names to stay consistent, which would mean storing them in the manifest, but the semantics for that interacting with remove are unclear, so I decided against that for now as well.

Examples for nix list output:

New profile managed entirely by nix profile:

$ outputs/out/bin/nix profile list --profile ~/repos/experiments/test-profile
Name:               vim
Index:              0
Flake attribute:    legacyPackages.x86_64-linux.vim
Original flake URL: flake:nixpkgs
Locked flake URL:   github:NixOS/nixpkgs/842e90934a352f517d23963df3ec0474612e483c
Store paths:        /nix/store/6j38m8vm8gp9a8qpw3b7dj9g50x1w95n-vim-9.0.1562

Name:               autojump
Index:              1
Flake attribute:    legacyPackages.x86_64-linux.autojump
Original flake URL: flake:nixpkgs
Locked flake URL:   github:NixOS/nixpkgs/842e90934a352f517d23963df3ec0474612e483c
Store paths:        /nix/store/aj1hslpfsw2a6vx83wmc2q8pjsafzr70-autojump-22.5.3

Old profile generated by nix-env:

$ outputs/out/bin/nix profile list --profile /nix/var/nix/profiles/per-user/felix/profile-38-link
Name:               autojump
Index:              0
Store paths:        /nix/store/y7xy1cvnf1ad0by318ny6hr13dqaq6rm-autojump-22.5.3

Name:               bat
Index:              1
Store paths:        /nix/store/822la7lq7sc36m1d9l30xjl33j0iyyf1-bat-0.18.3

Name:               direnv
Index:              2
Store paths:        /nix/store/67lwbcd8wcfn0npi94dhwb16hq5mf9ab-direnv-2.28.0

Name:               fd
Index:              3
Store paths:        /nix/store/0a1xw59mzy0hwn1q7ncrmn83aamq7mkw-fd-8.2.1

In a profile managed by nix profile with old packages installed by nix-env with duplicate packages:

outputs/out/bin/nix profile list --profile ~/repos/experiments/old-test-profile
Name:               vim
Index:              0
Store paths:        /nix/store/r37bkiiin3xfmkjm98lgpybchr9gmm15-vim-9.0.1562

Name:               asdf-vm
Index:              1
Store paths:        /nix/store/ciyg3pml5msmvq8y2m24lflkkfldq1z0-asdf-vm-0.11.3

Name:               ripgrep
Index:              2
Flake attribute:    legacyPackages.x86_64-linux.ripgrep
Original flake URL: flake:nixpkgs
Locked flake URL:   github:NixOS/nixpkgs/842e90934a352f517d23963df3ec0474612e483c
Store paths:        /nix/store/rsyhr2viynf9smk0ipma9h7lx7v1hxs3-ripgrep-13.0.0

Name:               vim1
Index:              3
Flake attribute:    legacyPackages.x86_64-linux.vim
Original flake URL: flake:nixpkgs
Locked flake URL:   github:NixOS/nixpkgs/842e90934a352f517d23963df3ec0474612e483c
Store paths:        /nix/store/6j38m8vm8gp9a8qpw3b7dj9g50x1w95n-vim-9.0.1562

My own profile, managed declaratively inside a single, local flake:

$ outputs/out/bin/nix profile list
Name:               .dotfiles
Index:              0
Flake attribute:    packages.x86_64-linux.default
Original flake URL: path:/home/felix/.dotfiles
Locked flake URL:   path:/home/felix/.dotfiles?lastModified=1688827211&narHash=sha256-1IfyfZq3U8KDHMX2hvnPIIsHQqUUgoFcGWAw0HFgw74%3D
Store paths:        /nix/store/6rkydkpb56iavshjm2cafb341am4gcvd-ifreilicht-default-packages

To-Do:

  • Fix tests
  • Potentially fix formatting
  • Actually remove Index functionality
  • Fix nix profile remove not matching packages installed by nix-env

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command labels Jul 9, 2023
@iFreilicht iFreilicht marked this pull request as draft July 9, 2023 21:03
@iFreilicht iFreilicht force-pushed the profile-names-instead-of-index branch from 8658be2 to 3a40929 Compare July 9, 2023 21:43
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 9, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/flakes-as-a-unified-format-for-profiles/29476/16

max-privatevoid added a commit to privatevoid-net/nix-super that referenced this pull request Jul 10, 2023
@edolstra edolstra self-assigned this Jul 14, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-07-14-nix-team-meeting-minutes-71/30472/1

@blaggacao
Copy link
Contributor

a heuristic approach was used that generates the names when the profile manifest is loaded.

This heuristic, whatever it ends up being, is independently useful, i.e. for ecosystem commands such as treefmt.

@iFreilicht iFreilicht force-pushed the profile-names-instead-of-index branch 2 times, most recently from 0539674 to 0c4c043 Compare July 16, 2023 12:07
@iFreilicht iFreilicht marked this pull request as ready for review July 16, 2023 12:13
Copy link
Member

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

This works really well!

I was a bit confused at first why name wasn't incorporated into manifest.json, but I see what you're saying. It's not clear yet if all bases are covered correctly already in terms of naming. I don't see problems with your approach atm compared to what we already have.

I caught myself attempting to make 'this could be better for UX'-type suggestions I realized this PR is a steppingstone. It does the minimal to replace indexes with names for Nix profile. This is already a step in the right direction for nix profile UX 👍 Increments on this can be done in separate PRs I think.

Thanks for the work so far already!

src/nix/profile-upgrade.md Outdated Show resolved Hide resolved
src/nix/profile-list.md Show resolved Hide resolved
src/nix/profile.cc Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/flakes-as-a-unified-format-for-profiles/29476/17

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-flakes-is-an-experiment-that-did-too-much-at-once/32707/11

@iFreilicht iFreilicht force-pushed the profile-names-instead-of-index branch from e1a74d6 to 01ea317 Compare October 17, 2023 22:21
@iFreilicht
Copy link
Contributor Author

@bobvanderlinden if you could have another look, that would be great! I updated the documentation a bit as well. The only thing missing are the release notes now.

Copy link

@TLATER TLATER left a comment

Choose a reason for hiding this comment

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

I have one small painpoint to raise, and that is that I think users actually expect this interface:

$ nix profile install nixpkgs#vim
$ nix profile upgrade nixpkgs#vim
$ nix profile remove nixpkgs#vim

The feasibility of that is a different question (because unfortunately the meaning of nixpkgs is neither unique nor clear in all contexts), and either way this change is probably the first step to that.

@bobvanderlinden also mentions potential UX improvements they intended to suggest, and this is probably one of them, I agree this point should not block the PR at all, but I think it's worth mentioning for reference.

Otherwise, this is great, working with names rather than numbers is obviously more human-friendly. Uniqueness is meanwhile guaranteed with numeric postfixes, so as far as I'm concerned this is a clean improvement over the current state.

It's also a beautiful diff, by the way, this is a very clean PR :)

src/nix/profile.cc Show resolved Hide resolved
src/libutil/url-name.cc Outdated Show resolved Hide resolved
src/nix/profile.cc Outdated Show resolved Hide resolved
tests/functional/nix-profile.sh Show resolved Hide resolved
@iFreilicht iFreilicht force-pushed the profile-names-instead-of-index branch 2 times, most recently from 7fbb9c4 to c1f6b77 Compare October 18, 2023 19:22
@iFreilicht
Copy link
Contributor Author

It's also a beautiful diff, by the way, this is a very clean PR :)

Thank you, I make extensive use of rebase -i master and it feels amazing.

PR is ready for another review, I now added the the release notes as well and would say this is final and ready to merge in my book.

The one remaining discussion would have to be decided by a maintainer.

iFreilicht and others added 2 commits November 6, 2023 21:21
Users may select specific outputs using the ^output syntax or selecting
any output using ^*.

URL parsing currently doesn't support these kinds of output references:
parsing will fail.

Currently `queryRegex` was reused for URL fragments, which didn't
include support for ^. Now queryRegex has been split from fragmentRegex,
where only the fragmentRegex supports ^.
@bobvanderlinden
Copy link
Member

@edolstra @fricklerhandwerk could this get another look? What does this need to get approval/rejection? Or what does this need to speed up the reviewing process?

@szlend
Copy link
Member

szlend commented Dec 16, 2023

It kind of feels like nobody in the Nix team actually uses nix profile. I've been trying to advocate Nix in our company and I'm frankly embarrassed every time I have to explain to someone how to upgrade or remove a package. This PR is a small backwards compatible change and a massive UX improvement. A lot of thought and effort has been put into this and it would be a shame if it was abandoned because of lack of interest from the maintainers.

@fricklerhandwerk
Copy link
Contributor

@edolstra said to take a look this week

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-12-18-nix-team-meeting-minutes-113/37050/1

@edolstra edolstra merged commit 17c202b into NixOS:master Dec 21, 2023
8 checks passed
@bobvanderlinden
Copy link
Member

🥳 thanks for the review + merge!

@iFreilicht iFreilicht deleted the profile-names-instead-of-index branch December 21, 2023 17:54
@iFreilicht iFreilicht mentioned this pull request Dec 21, 2023
16 tasks
Comment on lines +1 to +20
#include "url.hh"
#include "url-parts.hh"
#include "util.hh"
#include "split.hh"

namespace nix {

/**
* Try to extract a reasonably unique and meaningful, human-readable
* name of a flake output from a parsed URL.
* When nullopt is returned, the callsite should use information available
* to it outside of the URL to determine a useful name.
* This is a heuristic approach intended for user interfaces.
* @return nullopt if the extracted name is not useful to identify a
* flake output, for example because it is empty or "default".
* Otherwise returns the extracted name.
*/
std::optional<std::string> getNameFromURL(const ParsedURL & url);

}
Copy link
Member

Choose a reason for hiding this comment

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

@iFreilicht I would appreciate if you could move this elsewhere. libutil should be just utilities and not define Nix-specific concepts like store paths or flake things. (In the "Domain-Driven Design" parlance, this is a "application layer" or "domain layer" concept, but libutil is supposed to be the "infrastructure layer".)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Could you maybe give me a hint where "elsewhere" could be?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I would put in src/libexpr/flake/ for now. There are other flakes things in there that don't strictly relate to evaluation already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, see #9655

savil added a commit to jetify-com/devbox that referenced this pull request Feb 1, 2024
## Summary

The latest nix version (2.20) changed how the nix profile output is
represented:

From the [release
notes](https://nixos.org/manual/nix/stable/release-notes/rl-2.20):

> nix profile now allows referring to elements by human-readable names
NixOS/nix#8678

> [nix
profile](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-profile)
now uses names to refer to installed packages when running
[list](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-profile-list),
[remove](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-profile-remove)
or
[upgrade](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-profile-upgrade)
as opposed to indices. Profile element names are generated when a
package is installed and remain the same until the package is removed.

> Warning: The manifest.nix file used to record the contents of profiles
has changed. Nix will automatically upgrade profiles to the new version
when you modify the profile. After that, the profile can no longer be
used by older versions of Nix.

and for `nix search`:
> Disallow empty search regex in nix search
[#9481](NixOS/nix#9481)

> [nix
search](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-search)
now requires a search regex to be passed. To show all packages, use ^.


TODOs:
- [x] update `nix.readManifest` to handle the new format
- [x] `nix search` requires a regex to be passed
- [x] manually test on nix < 2.20 on devbox.sh to verify the older nix
still works

Fixes #1767 

## How was it tested?

CICD should pass

Installed nix 2.20.1 locally and am using Devbox with it to add, remove
packages and run scripts and shell.

verified flake updating works:
1. `examples/flakes/remote`. Did `devbox shell`, dropped the `v0.43.1`
from process-compose flake, did `devbox update`, and verified that
`process-compose` now had the latest version (IIRC `0.80+`)
2. `examples/flakes/php`. Did `devbox shell`, edited the flake to drop
`ds` and did `devbox update`. Verified no `ds` in `php -m | grep ds`
lf- pushed a commit to lix-project/lix that referenced this pull request May 2, 2024
Based off of commit 257b768

Upstream-PR: NixOS/nix#8678
Co-authored-by: Felix Uhl <[email protected]>
Change-Id: Idcb7f6191ca3310ef9dc854197f7798260c3f71d
lf- pushed a commit to lix-project/lix that referenced this pull request May 2, 2024
These names are parsed from the URL provided for that package

Based off of commit 257b768

Upstream-PR: NixOS/nix#8678
Co-authored-by: Felix Uhl <[email protected]>
Change-Id: I76d5f9cfb11d3d2915b3dd1db21d7bb49e91f4fb
lf- pushed a commit to lix-project/lix that referenced this pull request May 4, 2024
Based off of commit 257b768

Upstream-PR: NixOS/nix#8678
Co-authored-by: Felix Uhl <[email protected]>
Change-Id: Idcb7f6191ca3310ef9dc854197f7798260c3f71d
lf- pushed a commit to lix-project/lix that referenced this pull request May 4, 2024
These names are parsed from the URL provided for that package

Based off of commit 257b768

Upstream-PR: NixOS/nix#8678
Co-authored-by: Felix Uhl <[email protected]>
Change-Id: I76d5f9cfb11d3d2915b3dd1db21d7bb49e91f4fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

nix profiles upgrade and remove have counter-intuitive argument
10 participants