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 0075] Declarative wrappers #75

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

doronbehar
Copy link
Contributor

@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/7

@doronbehar doronbehar force-pushed the declarative-wrappers branch from 5ee0675 to d75c73e Compare August 16, 2020 13:26
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

In my opinion we need some kind of passthru "tag" system (where the tags can have values) that is used in conjuction with a buildEnv that takes a list of functions that use these tags to decide how to perform the composition. Then, there will be functions for the various Python interpreters (although they're essentially all the same).

Anyway, we need to start somewhere and see what issues we run into. As I commented elsewhere, nobody here has the entire overview.


In order for `wrapGenric` to know all of this information about our packaged
libraries - the information about runtime env, we need to write in the
`passthru`s of these libraries, what env vars they need. Such information was
Copy link
Member

Choose a reason for hiding this comment

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

Limiting ourselves to environment variables is at times too limiting. Also, environment variables should be the implementation detail, although the variable names could be used as identifier.

Python example.

If you have a closure of packages, and there happens to be a Python application with Python libraries as dependencies, then that group of packages needs to be composed. If there is another Python application depending on other libraries, then that needs to be composed separately. At least, they are not allowed to share the same *PYTHONPATH. (Note one also should not use PYTHONPATH but that's a different discussion.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is another Python application depending on other libraries, then that needs to be composed separately. At least, they are not allowed to share the same *PYTHONPATH

I see this is all about ABI compatibility, and I understand the issue while I remember you mentioned this in NixOS/nixpkgs#85103 . I'm still unsure whether it's possible it'd happen, assuming our libraries are composed OK. @FRidh could you perhaps give a situation (with perhaps some rough code examples) to a situation where say Python2's and Python3's libraries will share the same *PYTHONPATH? It's important to note that the current design counts upon buildInputs and propagatedBuildInputs only.

Copy link
Member

Choose a reason for hiding this comment

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

Different Python version is one case, the other is simply applications requiring different versions of a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

This can happen for example when you have a python3 application that shells out to an python2 application.

Copy link
Member

@FRidh FRidh Aug 20, 2020

Choose a reason for hiding this comment

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

Currently we prevent some of these effects by unsetting NIX_PYTHONPATH when a Python program starts, thereby avoiding leakage. This problem with wrappers and environment variables should also be noted clearly in the RFC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I think I start to understand better the concerns. But still, this is not a "Drawback" of the idea, I mean, we still need to workaround such edge cases specifically in our current methods right? I agree though that it might be more complicated if someone is obliged to use wrapGeneric and not a wrap hook.

As an implementation detail, it may be worth noting that it'd be nice to make wrapGeneric able to "catch" such incompatible values in an environment variable.

rfcs/0075-declarative-wrappers.md Show resolved Hide resolved
- [issue 54278](https://github.com/NixOS/nixpkgs/issues/54278)
- [issue 39493](https://github.com/NixOS/nixpkgs/issues/39493)

`GDK_PIXBUF` compatibilities? I haven't investigated them to the details, so feel
Copy link
Member

Choose a reason for hiding this comment

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

Both of the issues related to gdk-pixbuf only happened for non-wrapped derivations and current wrapGAppsHook would already fix them.

The issue was, IIRC, that gdk-pixbuf NixOS module sets GDK_PIXBUF_MODULE_FILE environment variable, which was enabling binary gdk-pixbuf modules with ABI incompatible with the gdk-pixbuf the programs linked against. In one case it was caused by a bug that slipped through the ABI stability guarantees, in the other by running a program for i686 on x86_64.

Either way, it is somewhat orthogonal to wrappers. Only that we might want to always wrap graphical applications with librsvg gdk-pixbuf module.

- [issue 54278](https://github.com/NixOS/nixpkgs/issues/54278)
- [issue 39493](https://github.com/NixOS/nixpkgs/issues/39493)

`GDK_PIXBUF` compatibilities? I haven't investigated them to the details, so feel
Copy link
Member

Choose a reason for hiding this comment

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

Though there is one concern around GDK_PIXBUF_MODULE_FILE – the environment variable supports only a single path so we need to build the cache containing all the modules using gdk-pixbuf-query-loaders – the current hook actually does not actually do that! but it is required for some image formats to work in e.g. eog. We will need to take inspiration in the module.

@michaelpj
Copy link

In my opinion we need some kind of passthru "tag" system (where the tags can have values) that is used in conjuction with a buildEnv that takes a list of functions that use these tags to decide how to perform the composition.

This reminds me of @nbp's old proposal which took this to the extreme of delaying any construction of derivations until the last minute. This has the advantage of giving you access to all the information that goes into a derivation when composing things, like shoving everything in passthru.

Obviously this is a radical change (and the motivation in the previous proposal was quite different), but I think it would help here too.

@timokau
Copy link
Member

timokau commented Aug 20, 2020

This reminds me of @nbp's old proposal which took this to the extreme of delaying any construction of derivations until the last minute.

Which in turn sounds a lot like the Nix-lang 2.0 proposal. We shouldn't extend the scope of this RFC too much, but its at least somewhat related.

The question why wrappers are needed to begin with was not yet answered.
It's also typically not the preferred choice so acknowledge that.
@FRidh
Copy link
Member

FRidh commented Aug 20, 2020

Many derivations nowadays have multiple outputs. Will this RFC consider multiple outputs? If so, let's say we have a multiple-output derivation, and we only needed to wrap bin/ which happens to be in the "bin" output, but there are also other outputs, and one of those outputs is in extraOutputsToInstall. Do we expect that, through our wrapper derivation, that output is picked up and included as expected?

Add introduction for run-time dependencies and wrappers.
@Mic92 Mic92 added the status: open for nominations Open for shepherding team nominations label Aug 27, 2020
@doronbehar
Copy link
Contributor Author

Many derivations nowadays have multiple outputs. Will this RFC consider multiple outputs? If so, let's say we have a multiple-output derivation, and we only needed to wrap bin/ which happens to be in the "bin" output, but there are also other outputs, and one of those outputs is in extraOutputsToInstall. Do we expect that, through our wrapper derivation, that output is picked up and included as expected?

Since extraOutputsToInstall is a NixOS option, wrapped derivations should be handled by a NixOS module. Here's the vision I had for the far future regarding this:

Once declarative wrappers will get mature, we should make environment.systemPackages detect derivations that are wrapped with wrapGeneric. Then, if needed, make the module call wrapGeneric differently then how it is called in all-packages.nix while considering extraOutputsToInstall.

In general, wrapGeneric should symlink to a single derivation both meta.outputsToInstall and what's in extraOutputsToInstall.

@infinisil infinisil changed the title [RFC 0075]: Declarative wrappers [RFC 0075] Declarative wrappers Nov 16, 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/pre-rfc-module-system-for-wrappers-in-nixpkgs/42281/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.