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: add lib.getExe' function #247115

Closed

Conversation

gytis-ivaskevicius
Copy link
Contributor

@gytis-ivaskevicius gytis-ivaskevicius commented Aug 4, 2023

Description of changes

We need this function to encourage the efficiency of build artifacts. Issues with alternatives:

  • Current implementation of lib.getExe: Throws a warning on the majority of packages due to lack of meta.mainProgram. I consider it useless at this point.
  • "${lib.getBin pkgs.something}/bin/something: Verbose, nobody is going to bother checking if package has bin output (View comment below)
  • "${pkgs.something}/bin/something: In case of derivations with multiple outputs is less space efficient (View comment below)

Ideal implementation

While I do realize issues that may come up with old implementation of lib.getExe, I think we gain more from having it than from removing it. I think ideally we should revert the patch which adds builtins.trace on lib.getExe and encourage usage of this function

I am okay with leaving lib.getExe as it is right now, but for that, I'd like for meta.mainProgram to be a required attribute on almost all derivations

Alternative implementation

Someone needs to go over the packages and implement `meta.mainProgram where it is appropriate

Things done

Copied old implementation of lib.getExe and named it lib.getExe'

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

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Aug 4, 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/lib-getexe-best-practices-discussion-thread/31290/1

getExe' pkgs.mustache-go
=> "/nix/store/am9ml4f4ywvivxnkiaqwr0hyxka1xjsf-mustache-go-1.3.0/bin/mustache"
*/
getExe' = x:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative name:

Suggested change
getExe' = x:
guessExe = x:

@gytis-ivaskevicius gytis-ivaskevicius marked this pull request as draft August 4, 2023 13:04
@gytis-ivaskevicius
Copy link
Contributor Author

Not ready to be merged, needs to be exposed in lib/default.nix. Will do that once we figure out the name for the function. It might be unnecessary if we end up reverting e6e16bc11843

@gytis-ivaskevicius
Copy link
Contributor Author

gytis-ivaskevicius commented Aug 4, 2023

After not sure how many years I may only now find out about this feature:

nix-repl> toString oxygen
"/nix/store/hwiqax93mgl5fv0w0jycb87h2xvgz2f5-oxygen-5.27.6-bin"

nix-repl> :b oxygen

This derivation produced the following outputs:
  bin -> /nix/store/hwiqax93mgl5fv0w0jycb87h2xvgz2f5-oxygen-5.27.6-bin
  dev -> /nix/store/cl4dha602ljs6vry2ccw73mpr1kxmncx-oxygen-5.27.6-dev
  out -> /nix/store/54g6b8b44388kxqzm3w3qh345q9v2hzp-oxygen-5.27.6

nix-repl> toString acl
"/nix/store/7djfrlhxlfmdk2cdafhg6gx2ykqwf99n-acl-2.3.1-bin"

nix-repl> :b acl

This derivation produced the following outputs:
  bin -> /nix/store/7djfrlhxlfmdk2cdafhg6gx2ykqwf99n-acl-2.3.1-bin
  dev -> /nix/store/d2b85ipf9jz0mdphzkzipls7cgnsfj59-acl-2.3.1-dev
  doc -> /nix/store/lq027yz89vc2c28c3678kvm4d6d4mq8i-acl-2.3.1-doc
  man -> /nix/store/dq5hv904jlkc4dwfcjmpg0c88r644qf3-acl-2.3.1-man
  out -> /nix/store/k64mgy0pwn8vh0zdfafplbxpllavd1jm-acl-2.3.1

Whenever stringifying derivation it results in bin output if it is available. Where is this implemented? I assume its nix feature, is it documented anywhere?

I would like some confirmation on this but I don't think that I have much of a use for the current implementation of lib.getExe if that's the case. It makes me wonder what is the usecase for lib.getBin since one may just stringify the derivation

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Aug 4, 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/lib-getexe-best-practices-discussion-thread/31290/3

@figsoda
Copy link
Member

figsoda commented Aug 4, 2023

Whenever stringifying derivation it results in bin output if it is available.

I believe this is because bin is the first entry in outputs:

outputs = [ "bin" "dev" "out" ];

nix-repl> toString tzdata
"/nix/store/hwdckdgh3y73qravhmx7dmkv825zbipi-tzdata-2023c"

nix-repl> tzdata ? bin
true

@infinisil infinisil mentioned this pull request Aug 13, 2023
12 tasks
@infinisil
Copy link
Member

See also #248895

@Artturin Artturin closed this Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 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.

5 participants