Search improvements for --file and --expr #7668

dramforever · 2023-01-23T16:28:43Z

Motivation

The new nix search no longer works without flakes without some hacks. This PR fixes this so that this would work:

$ nix search --file '<nixpkgs>' '' hello-unfree

Still a slight mouthful, but now it's at least working as much as --file could.

Context

Discussed in: https://discourse.nixos.org/t/nix-search-broken/24447

A workaround is possible, which uses the special handling of legacyPackages to ignore evaluation errors, but it's pretty ugly and relies on a weird internal behavior:

$ nix search --impure --expr '{ legacyPackages.${builtins.currentSystem} = import <nixpkgs> {};}' '' hello-unfree

There are two main changes:

CmdSearch now recurses into recurseForDerivations = true if file || expr. For better error messages, top-level eval errors are not ignored.
InstallableAttrPath::getCursors now has a custom implementation that auto-calls the value and provides the full attribute path in the cursor (root->findAlongAttrPath so that is has a correct parent).

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
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

tomberek · 2023-01-23T17:26:20Z

src/nix/search.cc

@@ -181,15 +181,21 @@ struct CmdSearch : InstallableCommand, MixJSON
                else if (initialRecurse)
                    recurse();

-                else if (attrPathS[0] == "legacyPackages" && attrPath.size() > 2) {
+                else if (file || expr || (attrPathS[0] == "legacyPackages" && attrPath.size() > 2))


Suggested change

else if (file || expr || (attrPathS[0] == "legacyPackages" && attrPath.size() > 2))

else if (file || expr || (attrPath.size() > 2))

The "legacyPackages" restriction prevents ability to leverage the RecurseForDerivations logic in any other attribute path. This also helps match the behavior a few lines below, also removing the restriction.

I'm not sure about flake installables, so I left the behavior as before.

(I think this also matches the behavior of nix flake show?)

nixos-discourse · 2023-01-24T12:01:28Z

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

https://discourse.nixos.org/t/nix-search-broken/24447/12

This means in commands like "nix search -f '<nixpkgs>'" the input is treated like a 'legacyPackages' package set.

The 'nix search' command uses this function. Without this auto-calling it will fail to find any packages.

fricklerhandwerk · 2023-03-03T13:21:17Z

Triaged in the Nix team meeting:

We want to support the use case, and it's relevant for stabilising the new CLI. But the proposed solution is suboptimal. @Ericson2314 and @tomberek will propose an alternative approach and report back. Postponed until then.

Complete discussion

@thufschmitt: we want the fix since it relates to stabilising the CLI
@edolstra: the problem with this is that the syntax is nix search <flakeref> <search term> and it's not clear how it would fit with non-flakes search
- the proposal uses a dummy argument, and that's really ugly
- if someone comes up with an elegant way for taking singleton installables, we can adapt it
@tomberek: the implementation may not be what we want, but we agree that we want to support the use case
@Ericson2314, @tomberek: would like to propose a design
idea approved, postponed; @Ericson2314 and @tomberek will report back with a proposal

nixos-discourse · 2023-03-03T13:35:26Z

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

https://discourse.nixos.org/t/2023-03-03-nix-team-meeting-minutes-37/25998/1

dramforever · 2023-03-05T07:15:55Z

I'd like to clarify that the '' isn't really dummy, it's just 'search from root'. You can search any other sub-package-set with e.g. nix search --file '<nixpkgs>' python3Packages

thufschmitt · 2023-03-06T08:58:52Z

@dramforever would . instead of '' work? That would probably make more explicit the fact that it's the path from the expression root and so make the command a bit more intuitive

dramforever · 2023-03-06T11:18:59Z

. already works. Though I want to say it's a bit of a hack, since InstallableCommands defaults to ., which means 'current directory' for flakes, and it just seems to be hacked around for the --file/--expr case to mean 'root'. I couldn't remember where in the code it's done though.

A redesign would probably still be nice. In my fork-ish nix-dram I've had it changed to:

nix search [-i INSTALLABLE] <terms ...>

Though that's because nix-dram is based on a 'default flake' (see discussion in e.g. #4438), which -i can, well, default to.

flickerfly · 2023-10-28T17:02:01Z

I'm coping with this by using the following alias. Hopefully, that'll help those who, like me, are looking forward to this bug being fixed.

alias nixpkgs-search="nix --extra-experimental-features nix-command --extra-experimental-features flakes search nixpkgs"

tejing1 · 2023-10-29T18:49:32Z

I created a wrapper to implement the legacyPackages workaround, and never thought to link it here... but here it is, in case it's useful: https://github.com/tejing1/nix-search

dramforever · 2023-11-06T17:07:45Z

(misclicked, sorry)

tomberek · 2023-11-22T23:27:06Z

This is a problem to fix to make nix search usable in the non-flake case.

@dramforever would . instead of '' work? That would probably make more explicit the fact that it's the path from the expression root and so make the command a bit more intuitive

"." is better than the empty string, it is still not particularly appealing for general usage.

we had said this above, but I'm not clear on a good design yet, see below

if someone comes up with an elegant way for taking singleton installables, we can adapt it

The contention is this part of the installables semantics:

Options that change the interpretation of installables:

      · --expr expr
        
        Interpret installables as attribute paths relative to the Nix expression expr.
    
      · --file / -f file
        
        Interpret installables as attribute paths relative to the Nix expression stored in file. If file is
        the character -, then a Nix expression will be read from standard input. Implies --impure.

brainstorming:

nix search -f '<nixpkgs>' something is the desired syntax?
but now it becomes harder to say nix search -f '<nixpkgs>' inside-this-attr something'

This would require logic for file&&expr to be detected earlier than when the search results are processed. @dramforever this would mean I'd expect conditionals in installable.cc for this case.

conclusion

this PR seems to be a step in the right direction, supporting both "" and "." to make the workaround better.

tomberek

needs a rebase
confirmation about what behavior is affected
Tests look good (though may need more if other behavior is affected, or to detect it)
release notes
docs

tomberek · 2023-11-22T23:48:37Z

src/libcmd/installables.cc

@@ -505,6 +505,26 @@ struct InstallableAttrPath : InstallableValue

        return res;
    }
+
+    std::vector<ref<eval_cache::AttrCursor>> getCursors(EvalState &state) override


I don't know if the getCursors change will impact other commands, i suspect it would now do an autoCall where before it didn't. It might be desirable to do so, but it is not clear how much behavior this is overriding.

tomberek · 2024-02-21T19:21:18Z

Related: https://discourse.nixos.org/t/installables-cli-design/38601

dramforever requested review from edolstra and thufschmitt as code owners January 23, 2023 16:28

tomberek reviewed Jan 23, 2023

View reviewed changes

dramforever added 3 commits January 24, 2023 21:02

nix search: Recurse into --file and --expr

2f8a8a1

This means in commands like "nix search -f '<nixpkgs>'" the input is treated like a 'legacyPackages' package set.

Auto-call toplevel in InstallableAttrPath::getCursors()

48efe0c

The 'nix search' command uses this function. Without this auto-calling it will fail to find any packages.

Add tests for 'nix search -f' recursion and auto-call

0c89224

dramforever force-pushed the search-recurse branch from 87e06dc to 0c89224 Compare January 24, 2023 13:06

fricklerhandwerk added new-cli Relating to the "nix" command bug labels Mar 3, 2023

thufschmitt mentioned this pull request Mar 3, 2023

nix search with -f works poorly with "installable" semantics #5642

Open

thufschmitt assigned Ericson2314 Mar 3, 2023

tomberek self-assigned this Mar 3, 2023

fricklerhandwerk added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Mar 3, 2023

dramforever closed this Nov 6, 2023

dramforever reopened this Nov 6, 2023

This was referenced Nov 21, 2023

fetchTarball regression in nixUnstable? #4785

Closed

nix search with -f '<nixpkgs>' lacking attribute names / package names from results #5641

Open

tomberek requested changes Nov 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search improvements for --file and --expr #7668

Search improvements for --file and --expr #7668

dramforever commented Jan 23, 2023 •

edited by tomberek

Loading

tomberek Jan 23, 2023

dramforever Jan 23, 2023 •

edited

Loading

nixos-discourse commented Jan 24, 2023

fricklerhandwerk commented Mar 3, 2023

nixos-discourse commented Mar 3, 2023

dramforever commented Mar 5, 2023

thufschmitt commented Mar 6, 2023

dramforever commented Mar 6, 2023

flickerfly commented Oct 28, 2023

tejing1 commented Oct 29, 2023

dramforever commented Nov 6, 2023

tomberek commented Nov 22, 2023

tomberek left a comment

tomberek Nov 22, 2023

tomberek commented Feb 21, 2024

	else if (file \|\| expr \|\| (attrPathS[0] == "legacyPackages" && attrPath.size() > 2))
	else if (file \|\| expr \|\| (attrPath.size() > 2))

Search improvements for --file and --expr #7668

Are you sure you want to change the base?

Search improvements for --file and --expr #7668

Conversation

dramforever commented Jan 23, 2023 • edited by tomberek Loading

Motivation

Context

Checklist for maintainers

tomberek Jan 23, 2023

Choose a reason for hiding this comment

dramforever Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

nixos-discourse commented Jan 24, 2023

fricklerhandwerk commented Mar 3, 2023

nixos-discourse commented Mar 3, 2023

dramforever commented Mar 5, 2023

thufschmitt commented Mar 6, 2023

dramforever commented Mar 6, 2023

flickerfly commented Oct 28, 2023

tejing1 commented Oct 29, 2023

dramforever commented Nov 6, 2023

tomberek commented Nov 22, 2023

brainstorming:

conclusion

tomberek left a comment

Choose a reason for hiding this comment

tomberek Nov 22, 2023

Choose a reason for hiding this comment

tomberek commented Feb 21, 2024

dramforever commented Jan 23, 2023 •

edited by tomberek

Loading

dramforever Jan 23, 2023 •

edited

Loading