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 toApp #95615

Closed
wants to merge 1 commit into from
Closed

lib: add toApp #95615

wants to merge 1 commit into from

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Aug 16, 2020

Introduce the same logic as in nix run, but accessible as a nix
function.

Motivations for this change
  1. Since nix run will have that logic, I think it's good to also try using it inside of nixpkgs. It will force us to improve pname attributes so that they map to the default binary. And also expose weaknesses in the heuristic.

  2. There are a lot of times in nixpkgs where we have these patterns:

'''
  ${pkgs.bash}/bin/bash bla bla bla
'''

which can now be replaced with:

'''
  ${lib.toApp pkgs.bash} bla bla bla
'''

Not all of these can be replaced but there are ~5k of those in nixpkgs using this heuristic: $ rg '}/bin/' | wc -l

There was a previous attempt with some discussion a long time ago, which this replaces: #67392

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.

@zimbatm zimbatm mentioned this pull request Aug 16, 2020
10 tasks
Introduce the same logic as in `nix run`, but accessible as a nix
function.
@bjornfor
Copy link
Contributor

Sorry of this is inappropriate, but how about moving this into stdenv.mkDerivation and have the path verified (that it exists) at build time? Usage could be e.g. pkgs.bash.app.

@zimbatm
Copy link
Member Author

zimbatm commented Aug 16, 2020

That's an entirely reasonable question and also one that @domenkozar was making in his comment in the previous PR: #67392 (comment) (which also stalled the previous PR).

Assuming that lib.toApp gets merged today, it doesn't prevent to extend the heuristic (and port it back to nix run).

Previously I thought that build-time checking would necessarily require double the number of derivations. But your question actually gave me the idea that it could be possible to just make it an extra stdenv.mkDerivation attribute that can be verified in the checkPhase.

@roberth
Copy link
Member

roberth commented Sep 1, 2020

This seems to be about making script writing easier, but the flakes app feature doesn't seem to be intended for this. Instead it's intended for desktop apps (or similar?) and potentially sandboxing those.

To keep the terminology consistent, we need a different name for this function (or derivation attribute for that matter). I suggest command or program.

For both apps and derivations it makes sense to have a program attribute. So perhaps this should be toProgram and stdenv could provide a checked attribute named program.

command isn't a thing yet afaik, but I think it makes sense to have defaultCommand next to defaultApp, considering that some desktop apps come with a cli and both flake attributes have different needs wrt sandboxing.

A toApp function may make sense, to turn a derivation with a program into a flake app input instead: p: { type = "app"; program = inherit (p) program; }

@stale
Copy link

stale bot commented Jul 20, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 20, 2021
@Artturin
Copy link
Member

Artturin commented Aug 2, 2022

#167247

@Artturin Artturin closed this Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants