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

pythonPackages.poetry: Inherit package from top-level #77629

Closed
wants to merge 1 commit into from

Conversation

@adisbladis
Copy link
Member

@adisbladis adisbladis commented Jan 13, 2020

Motivation for this change

We're having two instances of this package at the moment.

I'm a bit skeptical of this PR myself as it mixes pythonPackages derivations and poetry2nix derivations.
OTOH the upstream officially sanctioned way of installing poetry is this method which the poetry2nix package mimics very closely so I think that this will be preferred.

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.
@adisbladis adisbladis requested review from FRidh and jbaum98 Jan 13, 2020
@adisbladis adisbladis requested a review from jonringer as a code owner Jan 13, 2020
@adisbladis adisbladis mentioned this pull request Jan 13, 2020
2 of 10 tasks complete
@FRidh
Copy link
Member

@FRidh FRidh commented Jan 13, 2020

Mixing packages from two different sets will cause trouble, we should not do that. The current status is confusing but inevitable if we want to allow poetry as build system in python-packages.nix.

@adisbladis
Copy link
Member Author

@adisbladis adisbladis commented Jan 13, 2020

@FRidh I just added this to my PR description:

I'm a bit skeptical of this PR myself as it mixes pythonPackages derivations and poetry2nix derivations.
OTOH the upstream officially sanctioned way of installing poetry is this method which the poetry2nix package mimics very closely so I think that this will be preferred.

@FRidh
Copy link
Member

@FRidh FRidh commented Jan 13, 2020

Poetry provides a custom installer that will install poetry isolated from the rest of your system by vendorizing its dependencies. This is the recommended way of installing poetry.

Their installer installs poetry with deps in a separate location. After that, you will manage your envs entirely using poetry, so there will be no mixing with packages provided differently.

Using pip to install poetry is also possible.
Be aware, however, that it will also install poetry's dependencies which might cause conflicts.

This is how we install things in python-packages.nix, all packages need to be compatible. This of course makes it very challenging due to the shear amount of dependencies poetry itself has. Ideally, the poetry build system would be separated from the poetry application, but from a UI POV that's exactly what is not done.

@adisbladis
Copy link
Member Author

@adisbladis adisbladis commented Jan 13, 2020

The current pythonPackages.poetry derivation is not useful for anything except as a build-system as it leaks dependencies in normal usage and barely even that since it has overrides in a let block in the derivation which leads to collisions.

The way poetry is vendored in Nixpkgs ensures no propagation of dependencies so there is no collisions with the rest of pythonPackages.

The only new problem I can think of that comes with this PR is that it potentially introduces a problem if a build imports poetry (as opposed to calling the poetry script).
This is already a lot better than the status quo.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.