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

[RFC] Declarative wrappers #85103

Closed
wants to merge 31 commits into from

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Apr 12, 2020

Motivation for this change

Create a new wrapper to rule them all! 😈

Now Seriously: This has been talked over since like for ever and now I'd like to present to you a Pure Nix function that creates a wrapper environment for any derivation you'll give it, using symlinkJoin. It does so by recursively searching for a derivation's inputs' for derivations with an attribute such as:

  passthru = {
    propagateEnv = {
      GST_PLUGIN_SYSTEM_PATH_1_0 = "%out%/lib/gstreamer-1.0";
    };
  };

This recursive search means, that you don't have to take care of orchestrating wrapGAppsHook and wrapQtAppsHook or others with stuff like this:

  preFixup = ''
    makeWrapperArgs+=("''${qtWrapperArgs[@]}")
  ''

I tested in this new function in wrapping picard, gucview (which is currently not wrapped but there's an open PR that wraps it the old way #84449) and arandr.

Here's an example of the environments (new lines inserted to help readability) created for guvcview with the old way and the new way:

OLD

GIO_EXTRA_MODULES=
'/nix/store/z7gzw2bb3ypvf1wq1fjjn5r45idfgg0g-dconf-0.34.0-lib/lib/gio/modules'${GIO_EXTRA_MODULES:+':'}$GIO_EXTRA_MODULES
GIO_EXTRA_MODULES=
'/nix/store/z7gzw2bb3ypvf1wq1fjjn5r45idfgg0g-dconf-0.34.0-lib/lib/gio/modules'${GIO_EXTRA_MODULES:+':'}$GIO_EXTRA_MODULES
GDK_PIXBUF_MODULE_FILE=
'/nix/store/18l8ly5235r94s2rr122i3cfichccxqn-librsvg-2.46.4/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache'
XDG_DATA_DIRS=
'/nix/store/nwksqszw0dakmizrrlql9pamcf39vnxg-gtk+3-3.24.14/share/gsettings-schemas/gtk+3-3.24.14
/nix/store/slmsmbyfgf1rqj9rp6s1y33cqaz8q203-gsettings-desktop-schemas-3.34.0/share/gsettings-schemas/gsettings-desktop-schemas-3.34.0
/nix/store/nwksqszw0dakmizrrlql9pamcf39vnxg-gtk+3-3.24.14/share/gsettings-schemas/gtk+3-3.24.14
/nix/store/slmsmbyfgf1rqj9rp6s1y33cqaz8q203-gsettings-desktop-schemas-3.34.0/share/gsettings-schemas/gsettings-desktop-schemas-3.34.0'${XDG_DATA_DIRS
+'
'}$XDG_DATA_DIRS
XDG_DATA_DIRS=
'/nix/store/92p57svcsqgwa5fgb8z9qg7m7r66nx7a-guvcview-2.0.6/share'${XDG_DATA_DIRS:+':'}$XDG_DATA_DIRS

NEW

GDK_PIXBUF_MODULE_FILE=
${GDK_PIXBUF_MODULE_FILE-'/nix/store/a7dwk7qamr2j7220rsfvn8w386b8a3lc-librsvg-2.48.0/lib/gdk-pixbuf-2.0/2.10.0
loaders.cache'}
GIO_EXTRA_MODULES=
'/nix/store/0byixyx6l833y9a7ah8ygbh4d4nfgcin-dconf-0.36.0-lib/lib/gio/modules'${GIO_EXTRA_MODULES:+':'}$GIO_EXTRA_MODULES
GI_TYPELIB_PATH='/nix/store/2chzii7gwmlcii9x73l0v3bl7lf2awvb-gtk+3-3.24.14/lib/girepository-1.0
/nix/store/ly22x5s6v91i840xkkj82dhblkfgl79v-atk-2.35.1/lib/girepository-1.0
/nix/store/kah2hha1zcbv0xvyq7fdx26gxd03002x-gdk-pixbuf-2.40.0/lib/girepository-1.0
/nix/store/xjg7kpw38mfaab2fsarll2d39nk06ha5-gsettings-desktop-schemas-3.36.0/lib/girepository-1.0
/nix/store/r4vrzisk85nfkmmafvvi348czb8l4p86-gobject-introspection-1.64.0/lib/girepository-1.0
/nix/store/y9sn0hll1m90zdi8bwa5126lmdkdp8i9-pango-1.44.7/lib/girepository-1.0
/nix/store/a7dwk7qamr2j7220rsfvn8w386b8a3lc-librsvg-2.48.0/lib/girepository-1.0
/nix/store/kah2hha1zcbv0xvyq7fdx26gxd03002x-gdk-pixbuf-2.40.0/lib/girepository-1.0
/nix/store/ly22x5s6v91i840xkkj82dhblkfgl79v-atk-2.35.1/lib/girepository-1.0
'${GI_TYPELIB_PATH:+':'}$GI_TYPELIB_PATH
XDG_DATA_DIRS=
'/nix/store/2chzii7gwmlcii9x73l0v3bl7lf2awvb-gtk+3-3.24.14/share/gsettings-schemas/gtk+3-3.24.14
/nix/store/xjg7kpw38mfaab2fsarll2d39nk06ha5-gsettings-desktop-schemas-3.36.0/share/gsettings-schemas/gsettings-desktop-schemas-3.36.0
/nix/store/y2fj89hq03nxdpc7bv7k9dpbc3f082ah-runtime-env/share
'${XDG_DATA_DIRS:+':'}$XDG_DATA_DIRS

As you can see, there are duplicates entries in the old wrapper which increases the chances of hitting obsurd issues such as what #84689 fixed.

What didn't works?

(EDIT) Everything I tested.

How would a user override a derivation wrapped this way?

Say all-packages.nix has:

  my-awesome-pkg = wrapGeneric (callPackage ../applications/my-awesome-pkg { }) { };

Assuming the user knows my-awesome-pkg is wrapped with wrapGeneric, he would need to use an overlay like this:

self: super:

{
  my-awesome-pkg = super.wrapGeneric (
    super.my-awesome-pkg.unwrapped.overrideAttrs(oldAttrs: {
      preFixup = ''
        overriding preFixup from an overlay!!
      '';
    })
  ) {};
}
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.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/declarative-wrappers-another-idea/6661/6

@eadwu
Copy link
Member

eadwu commented Apr 13, 2020

Any particular reason for using %...%? Is there's none, then can't we just use @...@ or $... since it's already used elsewhere instead of adding something completely new (to my knowledge)?

@doronbehar doronbehar requested a review from cole-h April 13, 2020 10:01
@cole-h
Copy link
Member

cole-h commented Apr 13, 2020

I'm much happier with the @...@ wrapping, but I'm interested in why you reached for that rather than just $output (or ${placeholder "output"} at the declaration site, if plain $output would be hard to substitute). While I don't have a strong opinion on this, I would certainly like it more if it was consistent with how we refer to outputs basically everywhere else (maybe this has already been discussed; please point me there if that's the case).

I'm also just curious about the lack of path separators (:) in the "NEW" example's GI_TYPELIB_PATH -- does bash treat ASDF=one:two:three the same as ASDF=one\ntwo\nthree?

@doronbehar
Copy link
Contributor Author

I'm also just curious about the lack of path separators (:) in the "NEW" example's GI_TYPELIB_PATH -- does bash treat ASDF=one:two:three the same as ASDF=one\ntwo\nthree?

No at all. I manually added new lines for the sake of readability. Now I edited my first comment to indicate that.

I'm much happier with the @...@ wrapping, but I'm interested in why you reached for that rather than just $output (or ${placeholder "output"} at the declaration site, if plain $output would be hard to substitute).

That's a good question. I guess I could use $output and substitute that instead of @out@ but I didn't want that to feel like a shell variable expansion as that's taken care eventually by a pure Nix function, not a shell snippet. As for using ${placeholder "output"}, that's a bit harder to explain. I'll illustrate:

Let's take picard which relies on qt5.qtbase. Currently, qt5.qtbase has:

    propagateEnv = {
      QT_PLUGIN_PATH = "@bin@/${qtPluginPrefix}";
    };

If I change it to:

    propagateEnv = {
      QT_PLUGIN_PATH = "${placeholder "bin"}/${qtPluginPrefix}";
    };

One of the colon separated values I get in the wrapped picard is:

/04f3da1kmbr67m3gzxikmsl4vjz5zf777sv6m14ahv22r65aac9m/lib/qt-5.12.7/plugins

Versus what I get now with @bin@:

/nix/store/m0ykwbwhp84xrwj2sk7nsac09qnwnh0w-qtbase-5.12.7-bin/lib/qt-5.12.7/plugins

I think the documentation of placeholder supports this observation:

Return a placeholder string for the specified output that will be substituted by the corresponding output path at build time. Typical outputs would be "out", "bin" or "dev".

Hence placeholder seems to be useful mostly for build time scripts, not Nix functions that deal with the output path outside the derivation.

While I don't have a strong opinion on this, I would certainly like it more if it was consistent with how we refer to outputs basically everywhere else (maybe this has already been discussed; please point me there if that's the case).

I'm up for consistency as well, but I think the way to refer to outputs eventually depends on the context - shell scripts use $outname and others use placeholder or just drv.outname. I think wrapGeneric is unique in this sense. Here's why:

To make overrides for derivations built with wrapGeneric possible, we have to put the per-derivation propagateEnv info in every derivation's definition, not inside wrapGeneric itself. But, it seems a derivation can't ever know what will be the resulting store-path with which other derivations will refer to it. Take qtbase for instance:

qtbase can request wrapGeneric to add something to QT_PLUGIN_PATH of the wrapper. But, qtbase can't know the answer to "what will be the full path of this directory once another derivation will want to refer it?".

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/any-way-to-get-a-derivations-inputdrvs-from-within-nix/7212/5

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/awesomewm-luamodules-apparently-not-taking-effect/8507/6

@FRidh
Copy link
Member

FRidh commented Aug 15, 2020

@doronbehar i can help you out with the RFC.

What I would like to have is a set of functions that describe what should be done. Then, there is a new buildEnv that considers this set of functions. In case of Python, the function would traverse the buildInputs in a certain way to construct environment variables in a certain way. This is what should also be done for other tools / vars. Eventually, the result can be put in a JSON file in the build directory, so that a hook can simply take the results and apply them.

@doronbehar
Copy link
Contributor Author

@doronbehar i can help you out with the RFC.

Awesome :). If you've read my draft, there's that big list of issues I collected during my busy semester. I'd like to filter those who may not be relevant and better explain what's the issue behind them and explain for each how declarative wrappers could be used.

Perhaps we can split the work on the RFC for "Motivation" vs "Design"?

a set of functions that describe what should be done

? Nix Functions?

This is what should also be done for other tools / vars

Definitely 👍.

Eventually, the result can be put in a JSON file in the build directory, so that a hook can simply take the results and apply them.

That's even better.

@FRidh
Copy link
Member

FRidh commented Aug 16, 2020

About the build instruction, see how the JSON could look for a single wrapper #95569.
I imagine we collect the instructions for all executables of a build in a single JSON file, and then have the wrappers be created.

@doronbehar
Copy link
Contributor Author

Thanks for working on this @FRidh . I've updated most of the Motivation part of the RFC draft at https://github.com/doronbehar/rfcs/blob/declarative-wrappers/rfcs/0075-declarative-wrappers.md . I'll update tomorrow regarding progress for the design and other sections.

@doronbehar
Copy link
Contributor Author

Official RFC live at NixOS/rfcs#75 .

linkCmds_ = builtins.trace "linkCmds is ${builtins.toJSON linkCmds}" linkCmds;
in
symlinkJoin {
name = "runtime-env";
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should inherit the name of the parent derivation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not always there's a single parent derivation, but certainly for most purposes, this would be nicer.

# Useful for debugging, not evaluated if not used.
linkCmds_ = builtins.trace "linkCmds is ${builtins.toJSON linkCmds}" linkCmds;
in
symlinkJoin {
Copy link
Member

@jtojnar jtojnar Aug 16, 2020

Choose a reason for hiding this comment

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

One disadvantage of symlinkJoin is that when absolute path is used in an entry point (e.g. desktop files, systemd services…), it will launch the unwrapped program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.. Extra commands should be at postBuild for that to work. I wonder whether it'd be worth the effort to write something generic, or make wrapGeneric accept a extraBuildCommands string.

let
# From some reason, pkg == knownPkg doesn't work :/ don't know
# why... Never the less this should be good enough
deeperPkgs = builtins.filter notInEncyclopedia (pkg.buildInputs ++ pkg.propagatedBuildInputs);
Copy link
Member

Choose a reason for hiding this comment

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

How does the neowrapper handle propagation boundaries when buildInputs and propagatedBuildInputs are the same?
For example, libinput depends on pyudev for its utilities but we do not want everything that links against libinput to have pyudev as dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libinput not only has a certain python environment in it's buildInputs, it also references this python env at ${libinput.bin}/libexec/libinput/libinput-measure-fuzz. Note it's the bin output, not the more widely linked against out output.

we do not want everything that links against libinput to have pyudev as dependency.

Agreed. The current design suffers from the inability to distinguish between libinput specified in a drv's buildInputs, vs libinput.out (and not libinput.bin) referenced by a drv. With the ability to distinguish between the two, wrapGenric should be capable of knowing not to add python related env vars to a wrapper, if it's only libinput.out that's referenced.

This issue naturally relates to the unresolved questions of the RFC.

},
*/
wrapOut ? {},
}:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you consider targetting wrappers in-scope? For example, fwupd needs to exclude EFI files from being wrapped; gjs only wants to wrap installed tests…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I to understand you are concerned that wrapGeneric can't be instructed to wrap only some executables and not others?

Copy link
Member

@jtojnar jtojnar Aug 17, 2020

Choose a reason for hiding this comment

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

Yes. Ideally, it should be able to handle multiple disjoint wrappers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely we need to have that ability and it shouldn't be that hard to implement. I still don't have an idea though as to how this interface should look like. I'll update this PR or the RFC once I'll think about something, suggestions are welcome.

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@doronbehar
Copy link
Contributor Author

Closing as the design we aim at now has been thoroughly changed and there's nothing much interesting here IMO.

@doronbehar doronbehar closed this Feb 24, 2021
@doronbehar doronbehar deleted the declarative-wrappers branch March 2, 2023 11:09
@doronbehar doronbehar restored the declarative-wrappers branch March 2, 2023 11:09
@doronbehar doronbehar deleted the declarative-wrappers branch June 8, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants