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

Add exec, eachSystem, mkApp functions #173006

Closed
wants to merge 3 commits into from

Conversation

gytis-ivaskevicius
Copy link
Contributor

@gytis-ivaskevicius gytis-ivaskevicius commented May 14, 2022

Description of changes

Added several helper functions.
eachSystem function is copied from flake-utils, this allows hundreds if not thousands of flakes to remove flake-utils as a dependency

This should help to test out this PR gytis-ivaskevicius/nix2vim@33cd854

exec function is used as part of mkApp, but mainly inteadead for cases as such:

  ocrScript = pkgs.writeShellScript "ocr.sh" ''
    ${exec pkgs.grim} -g "$(${exec slurp})" -t ppm - | ${exec tesseract} - - | ${wl-clipboard}/bin/wl-copy
    ${exec libnotify} "$(${wl-clipboard}/bin/wl-paste)"
  '';

Worth considering renaming exec to something like I or _ so it would look more like an operator. I personally would love such change but did not commit it in such a way to avoid people protesting outside my window

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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/864

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels May 14, 2022
@Artturin
Copy link
Member

lib.getExe was added in #167247 so exec can be dropped from this pr

@jtojnar
Copy link
Member

jtojnar commented May 14, 2022

Also see #123117, #124866, #155709, #171235, #171864.

@gytis-ivaskevicius
Copy link
Contributor Author

@Artturin Now that's useful 👍

@jtojnar I knew about a couple of them, thought it's about time to give it another shot due to following reasons:

  • At this point its actually difficult to find someone not using flakes
  • We literally started merging in flakes specific code to nixpkgs like /lib/flakes.nix
  • Flake specific RFC's are being created

So at this point, I vote for us to merge it since:

  • Helps the users by reducing amount of dependencies they need to manage
  • In time it alleviates flakes inputs addressing (no longer need to overwrite flake-utils input for basically every single flake)
  • I can clearly see the community wanting this

mkApp libnotify "notify-send"
=> { type = "app"; program = "/nix/store/.../bin/notify-send"; }
*/
mkApp' = drv: binName: {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to combine those and fall back to the first argument of the second is not defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's possible to do it that way? The best I can think of in terms of merging is to swap arguments and check if the first argument is a derivation or a string, if its a string - return a function that accepts derivation

Copy link
Contributor Author

@gytis-ivaskevicius gytis-ivaskevicius May 14, 2022

Choose a reason for hiding this comment

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

Actually, another thought occurred - one argument that accepts either attribute set (same as flake-utils) or derivation. This would be a pretty clean implementation.

Anyways, let's wait and see what supreme codeowners has to say about all this, not sure what are your thoughts on the matter but I think its time for reviewers to give in and merge this or something similar

@blaggacao
Copy link
Contributor

blaggacao commented May 14, 2022

Fortunately, this is not about the merits of flake.

But about dropping one needless dependency for a lot of users that pollutes the dependency tree in the lockfile (roughly doubling its size).

So 👍

@adisbladis
Copy link
Member

I'm very 👎 on merging anything related to Flakes in the lib namespace.

Flakes are still an experimental feature. Merging anything related to them in lib only serves to ossify the current interface which still isn't standardised.

@sternenseemann
Copy link
Member

eachSystem function is copied from flake-utils, this allows hundreds if not thousands of flakes to remove flake-utils as a dependency

And here's me thinking flakes was about allowing to depend on Nix expressions outside of nixpkgs…

@adisbladis
Copy link
Member

With the same motivation as #173093 (comment) I'm closing this.
The lib namespace is not a place for experimental features.

@adisbladis adisbladis closed this May 15, 2022
@roberth
Copy link
Member

roberth commented May 15, 2022

Flakes are still an experimental feature. Merging anything related to them in lib only serves to ossify the current interface which still isn't standardised.

Please join NixOS/rfcs#82

But about dropping one needless dependency for a lot of users that pollutes the dependency tree in the lockfile

This really shouldn't matter. If the size of the lockfile already matters around this small number of dependencies, that would be indicative of a significant problem in the design of Flakes.

mkApp

This reads like you're constructing a deferred, inspectable function application or something. Might be my personal professional deformation though.

@roberth
Copy link
Member

roberth commented May 15, 2022

lib.getExe was added

I think we've considered exec, but it has a very specific meaning in POSIX that doesn't apply, and there's also some ambiguity with nix-exec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants