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.

Sign up for GitHub

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

Jump to bottom

command-not-found: drop perl dependency #74789

Closed
wants to merge 1 commit into from
Closed

command-not-found: drop perl dependency #74789

wants to merge 1 commit into from

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 1, 2019 •
edited
Loading

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

Mic92 commented Dec 1, 2019

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

Mic92
Mic92 commented Dec 1, 2019
View reviewed changes
nixos/modules/programs/command-not-found/src/command-not-found.cpp Outdated
}
}
} else if (getenv("NIX_AUTO_RUN")) {
std::string attrname = std::string("nixpkgs.") + package;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@Mic92 Mic92 force-pushed the command-not-found branch from 63c9a2e to e416c35 Compare December 1, 2019 15:27
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 1, 2019
@Mic92 Mic92 force-pushed the command-not-found branch from e416c35 to 3233ca3 Compare December 1, 2019 17:01
@Mic92 Mic92 requested a review from edolstra December 1, 2019 17:05
@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 commented Dec 2, 2019

Small typo in commit subject.

@Mic92 Mic92 force-pushed the command-not-found branch from 3233ca3 to d131ccc Compare December 3, 2019 10:51
@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2019

fixed

@edolstra
Copy link
Member

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.

edolstra
edolstra reviewed Dec 3, 2019
View reviewed changes
nixos/modules/programs/command-not-found/src/command-not-found.cpp Outdated
}
} else if (getenv("NIX_AUTO_RUN")) {
std::string attrname = std::string("nixpkgs.") + package;
std::vector<const char *> args = {"nix", "run", &attrname[0],
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Mic92 Mic92 Dec 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's say not safe instead of secure.

edolstra
edolstra reviewed Dec 3, 2019
View reviewed changes
nixos/modules/programs/command-not-found/src/command-not-found.cpp Outdated
}
};

int queryPackages(const char *system, const char *program,
Copy link
Member

Choose a reason for hiding this comment

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

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

edolstra
edolstra reviewed Dec 3, 2019
View reviewed changes
nixos/modules/programs/command-not-found/src/command-not-found.cpp Outdated

#ifndef NIX_SYSTEM
#error "NIX_SYSTEM not defined"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

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

@Mic92 Mic92 force-pushed the command-not-found branch from d131ccc to f012aa4 Compare December 3, 2019 19:57
@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2019 •
edited
Loading

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++:

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
Closed
10 tasks
@Mic92
command-not-found: drop perl dependency
2ccb9f5 
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 command-not-found branch from f012aa4 to 2ccb9f5 Compare December 6, 2019 10:34
@Mic92
Copy link
Member Author

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

Mic92 commented Dec 6, 2019 •
edited
Loading

I am fine with whatever the consensus is.

@doronbehar
Copy link
Contributor

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
Closed
10 tasks
@Mic92
Copy link
Member Author

Mic92 commented May 21, 2020

There is also a rust version alternative: #88517

@Mic92 Mic92 closed this Jun 23, 2020
@Mic92 Mic92 deleted the command-not-found branch November 1, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjornfor bjornfor bjornfor left review comments

@edolstra edolstra edolstra approved these changes

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mic92 @bjornfor @edolstra @doronbehar