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

python: propagate makeWrapper arguments #83321

Closed
wants to merge 2 commits into from

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Mar 24, 2020

Motivation for this change

Python libraries that have runtime dependencies, like Qt and GTK
libraries, are currently not working. These are not included in
the interpreter wrapper and no mechanism exist to do so automatically.

This commit adds a nix-support/make-wrapper-args file that stores
the makeWrapper arguments of a python derivation (created with
buildPythonPackage). The arguments are loaded from python.buildEnv
when creating the Python environment and passed to the makeWrapper
invocation.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change
  • Tested compilation of all pkgs that depend on this change: obviously not.
  • Tested the wrapping is working for pythonPackages.matplotlib with Qt
  • 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.

This should fix issues #25351 #80147 #39637

@rnhmjoj rnhmjoj requested a review from FRidh March 24, 2020 21:57
@rnhmjoj rnhmjoj requested a review from jonringer as a code owner March 24, 2020 21:57
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.

I think this is a good idea but want the bash code that's now in the postFixup somewhere else.

My idea is that the various wrapper scripts write to this make-wrapper-args file, until we have declarative wrappers. The consumer of that file should maybe be able to choose what parts to take based on an identifier, though I think we can do without. Let's wait for more feedback.

@@ -157,6 +157,19 @@ let
] ++ checkInputs;

postFixup = lib.optionalString (!dontWrapPythonPrograms) ''
# Propagate wrapper arguments up to the python
Copy link
Member

Choose a reason for hiding this comment

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

In mk-python-derivation a hook needs to do this; the function itself should remain small.

@worldofpeace @jtojnar any common approach that can be taken here? I want declarative wrappers in JSON with structured attributes, but we're not there. Let's discuss this in the main thread.

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 the various hook were to export their arguments in a common array, be it makeWrapperArgs or something else it would be great.

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 moved the shell code to a dedicated hook. I tested again with matplotlib and it seems good. I used a clone of buildPythonPackage to avoid a mass rebuild, so I haven't tested much else.

@rnhmjoj rnhmjoj closed this Apr 7, 2020
@rnhmjoj rnhmjoj deleted the python-wrapper branch April 7, 2020 12:42
@rnhmjoj rnhmjoj restored the python-wrapper branch April 7, 2020 12:42
@rnhmjoj rnhmjoj reopened this Apr 7, 2020
@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: policy discussion 6.topic: qt/kde 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 2.status: merge conflict This PR has merge conflicts with the target branch labels Apr 8, 2020
@rnhmjoj rnhmjoj removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 8, 2020
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 30, 2020
@ofborg ofborg bot requested review from FRidh and removed request for lovek323 August 30, 2020 08:35
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Aug 30, 2020

Rebased, fixed a conflict and removed the unnecessary escaping.

@FRidh
Copy link
Member

FRidh commented Aug 30, 2020

I have some changes lined up regarding this (https://github.com/FRidh/nixpkgs/pull/new/makewrapperargshook), but most importantly now, this PR breaks python3.tests. Every interpreter has a tests attribute containing some environment tests.

@rnhmjoj rnhmjoj force-pushed the python-wrapper branch 2 times, most recently from d1f8fee to 5ad4b81 Compare August 30, 2020 15:06
@ofborg ofborg bot requested a review from FRidh August 30, 2020 17:29
Python libraries that have runtime dependencies, like Qt and GTK
libraries, are currently not working. These are not included in
the interpreter wrapper and no mechanism exist to do so automatically.

This commit adds a `nix-support/make-wrapper-args` file that stores
the makeWrapper arguments of a python derivation (created with
buildPythonPackage). The arguments are loaded from python.buildEnv
when creating the Python environment and passed to the makeWrapper
invocation.
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Sep 2, 2020

@FRidh can you push your changes here? I couldn't use git am to pull them in because the hash of the ancestor changed while rebasing.

@FRidh
Copy link
Member

FRidh commented Nov 6, 2020

My apologies for not following up with this. I understand the need for this feature, but I am not happy with the eventual approach.
I am of the opinion the specific hooks should propagate a generic hook that does the final wrapping, and to get there that takes more effort. I started this off in #102949. Once that is in, it would be trivial to add this feature.

Declarative wrappers is just so much more work, that is definitely not something to wait for.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 6, 2020

No problem, I very well knew this approach wasn't the ultimate solution. I hoped it could be used as a stop gap to quickly fix python packages with Qt dependencies, but it seems there isn't much of an interest: there are only a couple open issues in Nixpkgs.

Thank you for starting the work on unifying the wrapper hooks. There was much talk about this situation but nothing material had been proposed.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/low-effort-and-high-impact-tasks/21095/41

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

Successfully merging this pull request may close these issues.

7 participants