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

Conversation

Mic92
Copy link
Member

@Mic92 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.

@domenkozar
Copy link
Member

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
Member 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

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
Member 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

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
Member 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

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
Member 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
Member 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 puzzlewolf left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Mic92 commented May 24, 2020

Anything else?

Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

Just two things :)

nixos/modules/programs/command-not-found/rust/src/main.rs Outdated Show resolved Hide resolved
@edolstra
Copy link
Member

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
Member 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.

Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

What do you think about backporting this in 20.03?

@edolstra
Copy link
Member

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

@Mic92
Copy link
Member Author

Mic92 commented May 25, 2020

Agreed. This should be not part of 20.03.

@puzzlewolf
Copy link
Contributor

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

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

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

nixos/modules/programs/command-not-found/default.nix Outdated Show resolved Hide resolved
nixos/modules/programs/command-not-found/rust/src/main.rs Outdated Show resolved Hide resolved
@Mic92
Copy link
Member 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...

@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)

- drops perl + libraries dependencies

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@Mic92
Copy link
Member 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
Member Author

Mic92 commented Jun 5, 2020

All issues resolved from point of view.

@Mic92
Copy link
Member 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 command-not-found-rust branch June 23, 2020 00:19
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/why-isnt-there-an-official-built-in-way-to-find-what-package-provides-a-specific-executable/22937/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet