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 search: Disallow empty regex #9481

Merged

Conversation

iFreilicht
Copy link
Contributor

Motivation

This will make invocations of nix search without a regex fail with the following error message:

nix search .
error: Must provide at least one regex! To match all packages, use '^'.
Try '/Users/feuh/repos/nix/outputs/out/bin/nix --help' for more information.

This protects users from accidentally printing every package in nixpkgs. If someone still wanted to do that, they can by explicitly passing ^. Why we recommend this over ^ is explained in the docs.

Context

Fixes #4739.

Also fixes #3553 in spirit IMO. That issue calls for outputting the entire output of nix search --help when no regex is passed, but that would be a more substantial change, as all new CLI commands have no short help and --help opens the manpage. If we wanted to change that, we should change it for all UsageError invocations.

I considered adding an --all flag, but this seemed redundant. I briefly looked into other ways of enforcing at least one argument with expectArgs, but adding optional = false didn't change the rest of the implementation or the docs, so I opted not to touch that part.

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 Nov 27, 2023
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.

This is a strict improvement in my opinion.

outputting the entire output of nix search --help when no regex is passed, but that would be a more substantial change

That would be a really cool change though.

@roberth
Copy link
Member

roberth commented Nov 27, 2023

Took a while for me to realize that ^ for outputs and ^ in a regex are very different things.
I guess it could confuse others to see a lone ^ in a place where you often find installables.
Maybe I'm overthinking this.

@iFreilicht iFreilicht force-pushed the disallow-nix-search-without-search-terms branch from 9c2dfe9 to 1499e6e Compare November 28, 2023 08:32
@iFreilicht
Copy link
Contributor Author

@roberth Good point! I added a note about this to the description.

Also added release notes, this is ready to merge from my side.

src/nix/search.md Outdated Show resolved Hide resolved
@iFreilicht iFreilicht force-pushed the disallow-nix-search-without-search-terms branch from 1499e6e to e07020a Compare November 28, 2023 19:08
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

I'm coming back to this and the error message is still confusing. I can imagine a user then trying "nix search ^". The key piece of info is that nixpkgs is not the default flake(ahem: dram), and that they need to specify it, assuming that is the most commonly searched flake. That there needs to be more positional arguments.

@iFreilicht
Copy link
Contributor Author

That's a good point. Ideally, the message would look like this:

$ nix search .
error: Must provide at least one regex! To match all packages, use 'nix search . ^'.
Try '/Users/feuh/repos/nix/outputs/out/bin/nix --help' for more information.

But this also has to work for all other kinds of installables (like file and expr), and after trying for over an hour now, I just can't make it happen. The easiest way is to use installable->what(), but that makes the message very confusing again:

$ nix search .
error: Must provide at least one regex! To match all packages, use 'nix search git+file:///Users/feuh/repos/nix#packages.aarch64-darwin ^'.

Or for files:

$ nix search -f tests/functional/search.nix 'foo'
error: Must provide at least one regex! To match all packages, use 'nix search foo ^'.

I would need some function that returns the installable verbatim, or at least close to verbatim. That would be useful for other error messages as well. Maybe I'll have another crack later.

@iFreilicht iFreilicht force-pushed the disallow-nix-search-without-search-terms branch from e07020a to d0231de Compare December 21, 2023 21:02
@iFreilicht iFreilicht force-pushed the disallow-nix-search-without-search-terms branch from d0231de to 397cf4e Compare December 21, 2023 21:13
@iFreilicht
Copy link
Contributor Author

@tomberek I now settled on this:

$ nix search .
error: Must provide at least one regex! To match all packages, use 'nix search <installable> ^'.

I don't know if it's a strict improvement over just ^, as <installable> introduces potential confusion again.

Please tell me which variant you like better, and then let's get that merged. Making a function to get the installable verbatim proved to be even more difficult than I thought, and I don't want to block a simple UX improvement like this on a big, possibly controversial addition.

Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

I do like the updated message, thx.

@tomberek tomberek merged commit 1c260fa into NixOS:master Jan 24, 2024
8 checks passed
@iFreilicht iFreilicht deleted the disallow-nix-search-without-search-terms branch January 24, 2024 06:57
@edolstra
Copy link
Member

edolstra commented Jan 24, 2024

Not really in favor of this. nix search nixpkgs seems to me like a perfectly logical and convenient way to show the contents of Nixpkgs (though it should be piped into the pager). Why make the user add unnecessary arguments?

@iFreilicht
Copy link
Contributor Author

I guess I should've seen that coming after #4809 was declined.

I disagree with "perfectly logical". I would expect a command like nix list or nix show to show a list of the contents of something. "convenient", sure. Once you know it, and as long as you use nix often enough to remember it.

I would appreciate concerns like this being raised before a contribution is merged, but whatever.
It would also help if issues that are not supposed to be fixed were just closed instead of leaving them open forever so I don't spend my limited time on stuff that is not gonna get merged anyway.

@tomberek
Copy link
Contributor

My rationale: It is a common problem for an initial user that they will often type "nix search python" and become bewildered. This helps address that by teaching them what they messed up. I agree it is convenient to automatically scan for everything, but the cost of confusion for beginners is too high for the slight benefit.

@iFreilicht
Copy link
Contributor Author

I mean I concede it's not a perfect fix. Right now:

$ nix search python
error: cannot find flake 'flake:python' in the flake registries

With this PR:

$ nix search python
error: Must provide at least one regex! To match all packages, use 'nix search <installable> ^'.

I would like it to look more like this

$ nix search python
error: Must provide at least one regex!
       To match all packages in the flake 'flake:python', use 'nix search python ^'.
       To search for 'python' in the 'nixpkgs' flake, use 'nix search nixpkgs python'.

But right now, it's seemingly impossible to get the initial arguments that were passed to a command inside InstallableValueCommand or any similar class like that, and my attempts at implementing this seemed to yield very ugly solutions.

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`
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
…hout-search-terms

nix search: Disallow empty regex

(cherry picked from commit 1c260fa)
Change-Id: Iaaf3605c24a342fcb05d0b534a9f305533d3b5fa
@daniels
Copy link

daniels commented Jan 10, 2025

So, I just noticed this now, after happily searching with no regex for a long time. Not all flakes are nixpkgs, so a full listing is not that uncommon, and for #attribute-search it is even more useful to do an empty search.

This "fix" is really an ergonomic regression. At least on my keyboard. ^ requires a modifier to type and is also a dead key, meaning I have to type <space> <shift>-] <enter> <enter> instead of just <enter> after nix search nixpkgs. I don't know which newcomer would prefer that.

@iFreilicht
Copy link
Contributor Author

Not all flakes are nixpkgs, so a full listing is not that uncommon

Ah yes, we talked about that, but concluded that most invocations of nix search are with nixpkgs, so we optimize for the majority case here. There was also a discussion in the Nix Team that yielded an interesting point, namely that if Ctrl+C worked better, #4739 would not be an issue.

and for #attribute-search it is even more useful to do an empty search.

Do you have an example for what the usecase of this is? I tried it on a few of my flakes and it seems to be either useless as it only outputs a single line or errors because it tries to evaluate things that aren't derivations.

This "fix" is really an ergonomic regression. At least on my keyboard. ^ requires a modifier to type and is also a dead key, meaning I have to type -] instead of just after nix search nixpkgs. I don't know which newcomer would prefer that.

This is a very unique situation, and we just can't consider everyone's custom setup. On most keyboards, ^ is a simple two-key combination, either with Shift or Alt Gr. I'm in a similar boat myself as I use a custom keyboard with the US-Intl layout to be able to type german characters easily, and I just added a macro so Shift+6 actually types Shift+6, then space.

I think I tried to allow an explicit empty string "" as well, but AFAIR this was not as trivial as expected and so I left it out.

@daniels
Copy link

daniels commented Jan 10, 2025

Not all flakes are nixpkgs, so a full listing is not that uncommon

Ah yes, we talked about that, but concluded that most invocations of nix search are with nixpkgs, so we optimize for the majority case here. There was also a discussion in the Nix Team that yielded an interesting point, namely that if Ctrl+C worked better, #4739 would not be an issue.

Yes I saw that and agreed.

and for #attribute-search it is even more useful to do an empty search.

Do you have an example for what the usecase of this is? I tried it on a few of my flakes and it seems to be either useless as it only outputs a single line or errors because it tries to evaluate things that aren't derivations.

I use this to explore package groups, like what vim plugins are available, what Ruby gems are packaged for nix etc. When typing nix search nixpkgs#rubyPackages in my understanding I have already provided a search term, the extra required ^ is just extra typing and mental burden.

This "fix" is really an ergonomic regression. At least on my keyboard. ^ requires a modifier to type and is also a dead key, meaning I have to type -] instead of just after nix search nixpkgs. I don't know which newcomer would prefer that.

This is a very unique situation, and we just can't consider everyone's custom setup. On most keyboards, ^ is a simple two-key combination, either with Shift or Alt Gr. I'm in a similar boat myself as I use a custom keyboard with the US-Intl layout to be able to type german characters easily, and I just added a macro so Shift+6 actually types Shift+6, then space.

I don't think it's very unique at all. The location might differ (my example above is from a Swedish standard layout, though I used the ] to signal the location of the key where it would be on an English layout, the actual Swedish key used is ¨ unshifted) , but having ^ as a dead key is common across many layouts. It is used for accenting other characters after all. And that is the primary problem, even though adding the required argument at all is also a usability regression to me.

I think I tried to allow an explicit empty string "" as well, but AFAIR this was not as trivial as expected and so I left it out.

Yeah, my workaround that I have yet to internalize is to search for ., with a lot of green in the output ...

@daniels
Copy link

daniels commented Jan 10, 2025

Oh, and I also use attribute search to check the version of a package.

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
None yet
Development

Successfully merging this pull request may close these issues.

nix search without a term should error Make nix search without search terms show the help message
8 participants