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: drop perl dependency #74789

Closed
wants to merge 1 commit into from
Closed

Conversation

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 1, 2019

Faster startup and remove some perl packages from the default nixos closure.
Eventually we could remove perl completely from the default nixos closure.
It now also uses nix run when NIX_AUTO_RUN is set.

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 nix-review --run "nix-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.
Notify maintainers

cc @

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Dec 1, 2019

I tested export NIX_AUTO_INSTALL=1, export NIX_AUTO_RUN=1 and without any of those.

}
}
} else if (getenv("NIX_AUTO_RUN")) {
std::string attrname = std::string("nixpkgs.") + package;

This comment has been minimized.

@Mic92

Mic92 Dec 1, 2019
Author Contributor

Is nixpkgs. correct here or should it be nixos.?

This comment has been minimized.

@edolstra

edolstra Dec 5, 2019
Member

nixpkgs., however it will become nixpkgs# in the future.

@Mic92 Mic92 force-pushed the Mic92:command-not-found branch from 63c9a2e to e416c35 Dec 1, 2019
@Mic92 Mic92 force-pushed the Mic92:command-not-found branch from e416c35 to 3233ca3 Dec 1, 2019
@Mic92 Mic92 requested a review from edolstra Dec 1, 2019
@Mic92 Mic92 changed the title commnand-not-found: drop perl dependency command-not-found: drop perl dependency Dec 1, 2019
@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Dec 2, 2019

Small typo in commit subject.

@Mic92 Mic92 force-pushed the Mic92:command-not-found branch from 3233ca3 to d131ccc Dec 3, 2019
@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Dec 3, 2019

fixed

@edolstra
Copy link
Member

@edolstra edolstra commented Dec 3, 2019

This to me is not really an improvement: it replaces a 51-line Perl script with a 141-line C++ program. As an aside, if we really want to get rid of Perl, then Rust is the way to go IMHO.

BTW, I think we can get rid of the auto-install feature, it was added as a bit of a joke and nobody uses it.

}
} else if (getenv("NIX_AUTO_RUN")) {
std::string attrname = std::string("nixpkgs.") + package;
std::vector<const char *> args = {"nix", "run", &attrname[0],

This comment has been minimized.

@edolstra

edolstra Dec 3, 2019
Member

nix run should be avoided, it's experimental and won't work in Nix 2.4 unless you pass --experimental-features nix-command.

This comment has been minimized.

@Mic92

Mic92 Dec 3, 2019
Author Contributor

@edolstra is this going away in future or when does it become stable?

This comment has been minimized.

@bjornfor

bjornfor Dec 3, 2019
Contributor

FYI, nix run is used a few places in nixpkgs (and the manual) already.

This comment has been minimized.

@Mic92

Mic92 Dec 3, 2019
Author Contributor

Not only nix run also nix copy and nix build (for example in the installer).

This comment has been minimized.

@edolstra

edolstra Dec 5, 2019
Member

Yeah, we should replace those by nix-build etc. (or add --experimental-features nix-command to those invocations, but that's tricky because it could override the user's experimental-features setting).

This comment has been minimized.

@edolstra

edolstra Dec 5, 2019
Member

BTW, maybe we should get rid of NIX_AUTO_RUN as well, automatically downloading/running binaries is not a very secure thing to do...

This comment has been minimized.

@Mic92

Mic92 Dec 6, 2019
Author Contributor

Let's say not safe instead of secure.

}
};

int queryPackages(const char *system, const char *program,

This comment has been minimized.

@edolstra

edolstra Dec 3, 2019
Member

These char * are a bit unidiomatic C++ IMHO.


#ifndef NIX_SYSTEM
#error "NIX_SYSTEM not defined"
#endif

This comment has been minimized.

@edolstra

edolstra Dec 3, 2019
Member

These are unnecessary, compilation will fail anyway if they're not defined.

@Mic92 Mic92 force-pushed the Mic92:command-not-found branch from d131ccc to f012aa4 Dec 3, 2019
@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Dec 3, 2019

This to me is not really an improvement: it replaces a 51-line Perl script with a 141-line C++ program. As an aside, if we really want to get rid of Perl, then Rust is the way to go IMHO.

BTW, I think we can get rid of the auto-install feature, it was added as a bit of a joke and nobody uses it.

I choose C++:

  • because it was proposed here: #48378 (comment)
  • it was accepted here: #68193
  • the sqlite interface is already in C, which made the building part quite simple and avoids having to cross-compile rust, when cross-compiling NixOS.

I am not sure the Rust version would be much shorter. The C++ is now 99 lines compared to 55 lines Perl. But it also generates more error messages and starts faster.
I guess I just wait for nix run to become stable - it is better suited for this use case since nix-shell exports all tons of unneeded environment variables and takes more time to execute as it spawns a shell with setup hooks.

@FRidh FRidh mentioned this pull request Dec 4, 2019
2 of 10 tasks complete
Faster startup and remove some perl packages from the default nixos closure.
Eventually we could remove perl completely from the default nixos closure.
@Mic92 Mic92 force-pushed the Mic92:command-not-found branch from f012aa4 to 2ccb9f5 Dec 6, 2019
@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Dec 6, 2019

Old closure size: 82.9M
New closure size: 33.6M

New one is faster but it probably hardly matters:

Old:

$ time command-not-found sqlite3
0.03s user 0.00s system 98% cpu 0.034 total

New:

$ time command-not-found sqlite3
0.00s user 0.00s system 87% cpu 0.004 total

The C++ program is now +28 lines longer.

Does this looks fine now?

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Dec 6, 2019

-1 on removing NIX_AUTO_INSTALL, since I'm using it. Or at least remove it in a separate PR with a release note entry.

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Dec 6, 2019

I am fine with whatever the consensus is.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented May 21, 2020

BTW for the record, nix-index (Rust written) has a database and a "command-not-found" shell function which can be used:

https://github.com/bennofs/nix-index#usage-as-a-command-not-found-replacement

Though from my experience it feels somewhat slower to that of the original command-not-found implementation.

@Mic92 Mic92 mentioned this pull request May 21, 2020
3 of 10 tasks complete
@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented May 21, 2020

There is also a rust version alternative: #88517

@Mic92 Mic92 closed this 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

4 participants
You can’t perform that action at this time.