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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul nix flake update and nix flake lock UX #8817

Merged
merged 4 commits into from Oct 31, 2023

Conversation

iFreilicht
Copy link
Contributor

@iFreilicht iFreilicht commented Aug 12, 2023

Motivation

The current interface of nix flake lock and nix flake update is surprising and verbose. This lead to #5110, which has been open for over two years and is currently the fourth-most upvoted issue in this repository.

After my proposal, some comments from users and a dedicated discussion from the Nix Team itself, this is the PR to fix this. The changes are as follows (copied directly from the release notes):

  • nix flake lock only creates lock files and adds missing inputs now. It will never update existing inputs.

  • nix flake update does the same, but will update inputs.

    • Passing no arguments will update all inputs of the current flake, just like it already did.
    • Passing input names as arguments will ensure only those are updated. This replaces the functionality of nix flake lock --update-input
    • To operate on a flake outside the current directory, you must now pass --flake path/to/flake.
  • The flake-specific flags --recreate-lock-file and --update-input have been removed from all commands operating on installables. They are superceded by nix flake update.

Testing

If you want to try out this PR, you can temporarily install it by running these two commands:

nix build github:iFreilicht/nix/flake-update-lock-overhaul
alias nix="$PWD/result/bin/nix --extra-experimental-features 'nix-command flakes'"

After that, running nix flake lock and nix flake update will use the most current version of this PR.

Context

See the links above for context.

My changes are pretty non-invasive. The only thing changed in the underlying lockFlake function are comments referring to the removed --update-input flag and a tiny bugfix that caused nix flake lock to not actually output what it added to the lockfile when initially creating it. The code examples for both nix flake lock and nix flake update were also incorrect, though those fixes are superseded by the new docs anyway.

You'll also see a note in both docs that refers to a potential tripping hazard in the way nix flake lock and nix flake update handle the flake-url argument:

Note: When trying to refer to a flake in a direct descendant directory, write ./another
instead of another, else Nix will try look up the flake in the registry.

To me this is clearly a bug, but I understand that the generality of flakes might make a seemingly obvious fix controversial, and I don't want this PR to be blocked because of that, so I'd be happy to create a separate PR for this issue.

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 with-tests Issues related to testing. PRs with tests have some priority labels Aug 12, 2023
@iFreilicht iFreilicht changed the title Flake update lock overhaul Overhault nix flake update and nix flake lock UX Aug 12, 2023
@iFreilicht iFreilicht changed the title Overhault nix flake update and nix flake lock UX Overhaul nix flake update and nix flake lock UX Aug 12, 2023
@edolstra edolstra self-assigned this Sep 1, 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-09-01-nix-team-meeting-minutes-84/32466/1

@iFreilicht
Copy link
Contributor Author

iFreilicht commented Oct 23, 2023

Rebased on current master and fixed all tests. It's ready to merge, just needs a review.

EDIT: Huh, seems the completion test passes on macOS and fails on Linux. Will investigate later.

@iFreilicht
Copy link
Contributor Author

Alright, tests are fixed! This was a bit of a tough one, see the commit message for more info.

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks for the nudge @iFreilicht. I actually had a WIP review but never finished it 馃槵

Apart from a few minor things, this is good to go from my side!

@edolstra you were assigned to this PR. Do you want to have another look? Otherwise I'll just merge it next week.

src/libutil/args.cc Show resolved Hide resolved
src/nix/flake-lock.md Outdated Show resolved Hide resolved
src/nix/flake.cc Outdated Show resolved Hide resolved
src/nix/flake.cc Outdated Show resolved Hide resolved
@iFreilicht iFreilicht force-pushed the flake-update-lock-overhaul branch 2 times, most recently from 0719c38 to 872cd1b Compare October 25, 2023 09:55
src/nix/flake-lock.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Docs LGTM. The non-blocking nitpick applies to two instances. Update if you have time and agree it's an improvement.

src/nix/flake-update.md Outdated Show resolved Hide resolved
iFreilicht and others added 4 commits October 31, 2023 15:33
Adding the inputPath as a positional feature uncovered this bug.
As positional argument forms were discarded from the `expectedArgs`
list, their closures were not. When the `.completer` closure was then
called, part of the surrounding object did not exist anymore.

This didn't cause an issue before, but with the new call to
`getEvalState()` in the "inputs" completer in nix/flake.cc, a segfault
was triggered reproducibly on invalid memory access to the `this`
pointer, which was always 0.

The solution of splicing the argument forms into a new list to extend
their lifetime is a bit of a hack, but I was unable to get the "nicer"
iterator-based solution to work.
@thufschmitt
Copy link
Member

Just rebased on top of master to fix a small conflict and set auto-merge. Should be good to go in 1h max once the CI is green.

Thanks a lot for that great change @iFreilicht !

@thufschmitt thufschmitt merged commit 12a0ae7 into NixOS:master Oct 31, 2023
8 checks passed
@9999years
Copy link
Contributor

@iFreilicht Thanks for your work getting this through!

@iFreilicht iFreilicht deleted the flake-update-lock-overhaul branch October 31, 2023 19:05
@chreekat
Copy link
Contributor

chreekat commented Nov 1, 2023

This is awesome!

@Ericson2314
Copy link
Member

This probably should have kept around the previous --update-input but deprecated. (While we are well within our rights to make sudden breaking changes to experimental features, when it is easy to leave up some back compat affordance we generally do so at least for a few releases. I think it would be easy to do so in this case?)

@iFreilicht Would you be willing to tackle this?

angerman added a commit to angerman/nix that referenced this pull request Nov 25, 2023
This builds on NixOS#8817, to add additional UX help for people with existing
muscle memory (or shell history) with --update-input and tries to gently
guide them towards the newly evolved CLI UI.
angerman added a commit to angerman/nix that referenced this pull request Nov 25, 2023
This builds on NixOS#8817, to add additional UX help for people with existing
muscle memory (or shell history) with --update-input and tries to gently
guide them towards the newly evolved CLI UI.

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
fricklerhandwerk pushed a commit that referenced this pull request Nov 27, 2023
This builds on #8817, to add additional UX help for people with existing
muscle memory (or shell history) with --update-input and tries to gently
guide them towards the newly evolved CLI UI.

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
github-actions bot pushed a commit that referenced this pull request Nov 27, 2023
This builds on #8817, to add additional UX help for people with existing
muscle memory (or shell history) with --update-input and tries to gently
guide them towards the newly evolved CLI UI.

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
(cherry picked from commit af00298)
fricklerhandwerk pushed a commit that referenced this pull request Nov 27, 2023
This builds on #8817, to add additional UX help for people with existing
muscle memory (or shell history) with --update-input and tries to gently
guide them towards the newly evolved CLI UI.

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
(cherry picked from commit af00298)
@bryango
Copy link
Member

bryango commented Jan 7, 2024

  • The flake-specific flags --recreate-lock-file and --update-input have been removed from all commands operating on installables. They are superceded by nix flake update.

Hey this is a nice refresh for nix flake, but it eliminates the possibility of doing the super convenient, single command

nix build --update-input nixpkgs

So should we now do

nix flake update nixpkgs && nix build

instead? 馃憖 If this is the final decision, I can learn to live with it.

@fricklerhandwerk fricklerhandwerk added the UX The way in which users interact with Nix. Higher level than UI. label Jan 8, 2024
@iFreilicht
Copy link
Contributor Author

iFreilicht commented Jan 9, 2024

@bryango Yes, that was the decision, and nobody objected at the time. Of course, this was not a data-driven decision, so I'd be curious to hear your use-case for --update-input. Maybe it's genuinely useful over the longer one-liner, though we are only talking about five extra keystrokes here.

@bryango
Copy link
Member

bryango commented Jan 9, 2024

Ah, there is not much that is "genuinely more useful" about --update-input. I do think that the old nix build --update-input nixpkgs fits better with my cognitive model: the main objective is nix build, and occasionally I will need to decorate that with some options, such as --update-input nixpkgs (this often comes as an afterthought to nix build).

Therefore, I would hope that the --update-input flag could be un-deprecated for commands such as nix build and nix develop. However, do note that this could be a very personal workflow (obligatory xkcd), and as mentioned before, I can live with the two-step commands as well, so this is not a protest 馃槅 just to share a possible common workflow. And, for nix flake I think the new interface is much nicer and I fully support the changes.

Update: oh, I just thought of something that could be "genuinely useful", but I am not sure if I have done it before, so this may or may not be realistic: suppose I want to try out some remote flake, but with some updated input; before I can do:

nix run github:some-remote-flake --update-input nixpkgs

... without polluting the working directory at all, but now I will have to clone the remote flake first, nix flake update nixpkgs, and finally nix run. Do note that this is an imaginary workflow; I am not sure if I have done it before.

@bryango
Copy link
Member

bryango commented Jan 11, 2024

suppose I want to try out some remote flake, but with some updated input; before I can do:

nix run github:some-remote-flake --update-input nixpkgs

... without polluting the working directory at all, but now I will have to clone the remote flake first, nix flake update nixpkgs, and finally nix run.

In light of this, I think it would be better to un-deprecate the flags (while keeping the improved nix flake behavior). Building on https://github.com/NixOS/nix/pull/9449/files one can simply remove the warn s for these flags. What do you think? @fricklerhandwerk @iFreilicht

@iFreilicht
Copy link
Contributor Author

Got it, thank you for your thoughts! That imaginary workflow is indeed very useful, and would currently be served by --override-input, though you'd have to write nix run --override-input nixpkgs nixpkgs, meaning you override the input nixpkgs with whatever flake nixpkgs resolves to locally, which could be a pinned version of nixpkgs, too.

What you show with --update-input doesn't actually work as far as I could tell, which is partly why I wanted to get rid of it; it only works for local flakes you have write access to, even though you would expect otherwise.

The other reason is that nix has an issue with too few maintainers for too many features. While the flakes interface is still experimental, it makes sense to remove some features in favor of others to keep it as small as possible. Having multiple ways of achieving the same task makes keeping consistency, testing and documenting more difficult. Your demonstrated usecase is a potential feature for the future, but when it is implemented, we have to ensure it works properly in all situations it is available in.

Right now, the priority regarding flakes is to stabilize them, so putting in the required amount of work to make this flag work as expected is not feasible, unless a contributor wants to take that over, and even then, finding time in a maintainers schedule can take months. So I would say deprecating and removing these flags is the best option right now. It says clearly:
"This might not work as expected, and we will remove it, not fix it."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation flakes new-cli Relating to the "nix" command UX The way in which users interact with Nix. Higher level than UI. 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.

None yet

9 participants