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

lib.getExe: Do not make assumptions about the main program #246386

Merged

Conversation

roberth
Copy link
Member

@roberth roberth commented Jul 31, 2023

Before this commit, getExe assumes that if meta.mainProgram is unset, it has a main program that's named after the package name.

While this is probable, it leads to a bad error when the assumption does not hold. If the user called getExe themselves, they might narrow down the location of the assumption quite easily, but if that's not the case, they'll have to go down the rabbit hole to figure out what went wrong.

For example, a NixOS module may use lib.getExe on a package-typed option which is then used in the system configuration. This then typically leads to a failure after deployment, which is bad, and it's quite likely that the user will debug the package output contents before digging through the NixOS module, which is bad.
Furthermore the getExe call is rather inconspicuous as it does not contain something like "/bin/foo", which is bad.
Also modules can be hard to read for a newbie, which is bad.

All of this can be avoided by requiring meta.mainProgram. Many packages already have the attribute, and I would expect 80% of getExe usages to be covered by 20% of packages, because plenty of packages aren't used with getExe anyway.

Finally we could make an effort to set mainProgram semi-automatically using nix-index.

Description of changes

Adds a warning to the unreliable behavior.

[nixpkgs]$ nix repl .
nix-repl> lib.getExe (hello // { meta = {}; })
trace: warning: getExe: Package hello does not have the meta.mainProgram attribute. We'll assume that the main program has the same name for now, but this behavior is deprecated, because it leads to surprising errors when the assumption does not hold. If the package has a main program, please set `meta.mainProgram` in its definition to make this warning go away. Otherwise, if the package does not have a main program, or if you don't control its definition, specify the full path to the program, such as "${lib.getBin foo}/bin/bar".
"/nix/store/nj2460zk9fjdl9yyglsi2nh4lp78na24-hello-2.12.1/bin/hello"

Context

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Before this commit, getExe assumes that if `meta.mainProgram` is unset,
it has a main program that's named after the package name.

While this is probable, it leads to a bad error when the assumption does
not hold. If the user called `getExe` themselves, they might narrow down
the location of the assumption quite easily, but if that's not the case,
they'll have to go down the rabbit hole to figure out what went wrong.

For example, a NixOS module may use `lib.getExe` on a package-typed option
which is then used in the system configuration. This then typically leads
to a failure *after* deployment, which is bad, and it's quite likely that
the user will debug the package output contents before digging through the
NixOS module, which is bad.
Furthermore the `getExe` call is rather inconspicuous as it does not
contain something like "/bin/foo", which is bad.
Also modules can be hard to read for a newbie, which is bad.

All of this can be avoided by requiring `meta.mainProgram`.
Many packages already have the attribute, and I would expect 80% of
`getExe` usages to be covered by 20% of packages, because plenty of
packages aren't used with `getExe` anyway.

Finally we could make an effort to set `mainProgram` semi-automatically
using `nix-index`.
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Jul 31, 2023
Unfortunately there's no test for me to confirm that it works,
so all I can do is ask for maintainers, unfortunately -- I mean...
This is your opportunity!
@roberth roberth requested a review from mweinelt as a code owner July 31, 2023 19:23
@github-actions github-actions 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/` labels Jul 31, 2023
@github-actions github-actions bot added 6.topic: python 6.topic: GNOME GNOME desktop environment and its underlying platform labels Jul 31, 2023
This should fix most warnings getExe in based on grepping `nixos/`.
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM and I support this.

@roberth roberth force-pushed the lib-getExe-dont-make-name-assumption branch from 68b6646 to e82c082 Compare July 31, 2023 20:32
@roberth roberth force-pushed the lib-getExe-dont-make-name-assumption branch from e82c082 to 0ed9e35 Compare August 1, 2023 10:21
@ofborg ofborg bot added 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Aug 1, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-init-generate-nix-packages-from-urls-with-hash-prefetching-dependency-inference-license-detection-and-more/25035/18

Artturin pushed a commit that referenced this pull request Sep 19, 2023
toastal pushed a commit to toastal/nixpkgs that referenced this pull request Sep 25, 2023
@jian-lin jian-lin mentioned this pull request Sep 28, 2023
12 tasks
@eclairevoyant eclairevoyant mentioned this pull request Oct 1, 2023
12 tasks
@eclairevoyant eclairevoyant mentioned this pull request Oct 6, 2023
12 tasks
arcnmx added a commit to arcnmx/nixpkgs that referenced this pull request Oct 28, 2023
@ambroisie ambroisie mentioned this pull request Nov 21, 2023
13 tasks
@drupol drupol mentioned this pull request Nov 29, 2023
13 tasks
arcnmx added a commit to arcnmx/nixpkgs that referenced this pull request Jan 18, 2024
@khaneliman khaneliman mentioned this pull request Mar 24, 2024
13 tasks
@fgaz fgaz mentioned this pull request Oct 30, 2024
13 tasks
@fabaff fabaff mentioned this pull request Jan 9, 2025
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: lib The Nixpkgs function library 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: clean-up 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: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants