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

command-not-found: rewrite in Rust #88517

Closed
wants to merge 1 commit into from
Closed

Conversation

@Mic92
Copy link
Contributor

Mic92 commented May 21, 2020

  • drops perl + libraries dependencies

Rust version of #74789 as @edolstra prefers Rust over C++: #74789 (comment)

Motivation for this change
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.
@Mic92 Mic92 requested review from edolstra and 2chilled and removed request for 2chilled May 21, 2020
@Mic92 Mic92 mentioned this pull request May 21, 2020
4 of 10 tasks complete
@domenkozar
Copy link
Member

domenkozar commented May 21, 2020

As we are slowly going to get rid of channels as a mean of distribution, it would be good to go towards something like https://github.com/bennofs/nix-index

Meanwhile command-not-found in rust will do :)

@Mic92
Copy link
Contributor Author

Mic92 commented May 21, 2020

Agreed. I am just not sure how nix-index can be integrated into NixOS smoothly because it requires a user to run it at least once.

@domenkozar
Copy link
Member

domenkozar commented May 21, 2020

A couple of ways I can think of:

a) encourage users to display it: Command 'foo' not found. Run nix-index to propose how to install it.

b) hook into user profiile generation and run it then, like we do for some things that already depend on whole profile

@emilazy
Copy link
Member

emilazy commented May 21, 2020

If moving towards a nix-index-style approach, it would be nice if the index for nixpkgs could be built on Hydra and cached so people don't have to do a 5 minute batch job to get command hints (consider e.g. the live CD use-case), though I guess that would probably require some thought towards splitting up the index to avoid large redownloads.

@Mic92
Copy link
Contributor Author

Mic92 commented May 21, 2020

Just for statistics:

~/.cache/nix-index/files is currently 27mb.
A uncompressed nixpkgs channel with is 101MB, where the programs.sqlite itself is 4MB.

@matthewbauer
Copy link
Member

matthewbauer commented May 21, 2020

nix-index has its own command-not-found btw: https://github.com/bennofs/nix-index/blob/master/command-not-found.sh

If moving towards a nix-index-style approach, it would be nice if the index for nixpkgs could be built on Hydra and cached so people don't have to do a 5 minute batch job to get command hints (consider e.g. the live CD use-case), though I guess that would probably require some thought towards splitting up the index to avoid large redownloads.

It's kind of awkward because nix-index relies on a completed Hydra jobset. We could probably set up a meta one just for nix-index though. Alternatively, we could go into the channel like programs.sqlite does (https://github.com/NixOS/nixos-channel-scripts/blob/master/generate-programs-index.cc). Unfortunately the db is pretty big (~30M) so I'm not sure if that's reasonable. See #39789

@Mic92
Copy link
Contributor Author

Mic92 commented May 21, 2020

I wonder if it is actually worth having a database or if it would not be more efficient to just have a compressed text file that is search through linear.

@doronbehar
Copy link
Contributor

doronbehar commented May 22, 2020

I'm up for switching to nix-index and all that, but you should note that (I'm using it for a while now) it's database lookup seems slower then the current implementation, I suspect it's due to the size of the database as it has paths for every file there is. Maybe we could modify nix-index a bit so it'll be able to generate a database of^/bin/ directories only?

@Mic92
Copy link
Contributor Author

Mic92 commented May 22, 2020

Maybe nix-index could accept multiple sources. Than we could ship one for bin by default and the other larger one has to be enabled explicitly.

@Mic92
Copy link
Contributor Author

Mic92 commented May 22, 2020

I opened an issue for nix-index. Please focus on code review of this rewrite only. Replacing it with nix-index is beyond the scope.

Copy link
Contributor

puzzlewolf left a comment

Here's a review for the Rust code :)

I don't read perl, so I didn't compare the two implementations, but it looks like the Rust code is doing the right thing 👍

Most remarks are stylistic changes, with the exception of the inline use of exit, where I'm not sure if it's actually bad :)

@Mic92 Mic92 force-pushed the Mic92:command-not-found-rust branch from 7ebd296 to 9f153b3 May 24, 2020
@Mic92
Copy link
Contributor Author

Mic92 commented May 24, 2020

Anything else?

Copy link
Contributor

puzzlewolf left a comment

Just two things :)

@edolstra
Copy link
Member

edolstra commented May 25, 2020

We should probably just get rid of command-not-found. It only works if you use the nixpkgs/nixos channels, and channels are on their way out.

@Atemu
Copy link
Member

Atemu commented May 25, 2020

When Flakes reach stable, yes, it should be removed but until then it remains as useful as ever and a clean rewrite is appreciated.

Though even if they came in 20.09 this tool would be useful for at least another 4-5 months because I don't think we'll get Flakes in 20.03.

@Mic92
Copy link
Contributor Author

Mic92 commented May 25, 2020

I would suggest when nix gets flakes to disable the command-not-found module and just use nix-index when enabled.

@Mic92 Mic92 force-pushed the Mic92:command-not-found-rust branch from 9f153b3 to 56aeca2 May 25, 2020
Copy link
Contributor

puzzlewolf left a comment

LGTM 👍

What do you think about backporting this in 20.03?

@edolstra
Copy link
Member

edolstra commented May 25, 2020

@puzzlewolf That kind of change definitely shouldn't be backported to a stable branch.

@Mic92
Copy link
Contributor Author

Mic92 commented May 25, 2020

Agreed. This should be not part of 20.03.

@puzzlewolf
Copy link
Contributor

puzzlewolf commented May 25, 2020

@edolstra Well, if you put it like that :D Sorry, don't know what i was thinking.

Copy link
Contributor

jtojnar left a comment

It also seems to remove NIX_AUTO_INSTALL and NIX_AUTO_RUN support, which I do not see mentioned.

@Mic92
Copy link
Contributor Author

Mic92 commented May 25, 2020

@edolstra asked me to remove NIX_AUTO_INSTALL and NIX_AUTO_RUN support in #74789 (comment) It could be re-added...

@Mic92 Mic92 force-pushed the Mic92:command-not-found-rust branch from 56aeca2 to 54dddbd May 26, 2020
@bennofs
Copy link
Contributor

bennofs commented Jun 4, 2020

I wonder if it is actually worth having a database or if it would not be more efficient to just have a compressed text file that is search through linear.

Well, that's basically what nix-index does. It just does some simple text processing before that to improve compression rates and search speed (the same thing that locate db does: sort and encode common prefixes using a simple "repeat N chars of last prefix" approach)

@Mic92 Mic92 force-pushed the Mic92:command-not-found-rust branch from 54dddbd to ef51e5c Jun 5, 2020
- drops perl + libraries dependencies

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@Mic92 Mic92 force-pushed the Mic92:command-not-found-rust branch from ef51e5c to edaece2 Jun 5, 2020
@Mic92
Copy link
Contributor Author

Mic92 commented Jun 5, 2020

I wonder if it is actually worth having a database or if it would not be more efficient to just have a compressed text file that is search through linear.

Well, that's basically what nix-index does. It just does some simple text processing before that to improve compression rates and search speed (the same thing that locate db does: sort and encode common prefixes using a simple "repeat N chars of last prefix" approach)

Basically it is just that the full database is a bit too large.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 5, 2020

All issues resolved from point of view.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 23, 2020

I just switched to nix-index given that command-not-found will die eventually.

@Mic92 Mic92 closed this Jun 23, 2020
@Mic92 Mic92 deleted the Mic92:command-not-found-rust branch Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.