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

poetry: don't use poetry2nix #209534

Merged
merged 5 commits into from
Jan 17, 2023
Merged

Conversation

dotlambda
Copy link
Member

@dotlambda dotlambda commented Jan 7, 2023

The use of poetry2nix within Nixpkgs is discouraged since we have a well-tested Python package set.

Description of changes
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/)
  • 23.05 Release Notes (or backporting 22.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.

@mweinelt
Copy link
Member

mweinelt commented Jan 8, 2023

We should probably move poetry out of python-modules and patch everything to use poetry-core. That way we can use packageOverrides to be more agile with poetrys dependencies.

It is a CLI tool for Python packaging. Python modules should use
poetry-core as a build-time dependency instead.
@dotlambda
Copy link
Member Author

@mweinelt beancount-ing-diba was the only remaining package using poetry instead of poetry-core

@dotlambda dotlambda marked this pull request as ready for review January 9, 2023 16:39
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

@adisbladis
Copy link
Member

If you want to to this (I'm totally 100% in disagreement with this PR) also remove me as a maintainer from the package as I want nothing to do with the python package sets.

@adisbladis
Copy link
Member

Also, this begs the question: What is even the purpose of having maintainers listed if you're going to continuously go against their wishes anyway? I've been clearly saying no to efforts like this in the past and @dotlambda is well aware of this.
But if you're going to keep steamrolling despite maintainer wishes I'm not gong to keep fighting this.
I'm sick and tired of this crap and quite frankly on a personal level it's extremely demotivating.

IMHO this is the exact opposite direction to what nixpkgs should go towards as I see the current Python maintenance model in nixpkgs as unsustainable, and for the lack of a better word to express it, insane.

Additionally, as I have express before this results in leaking all of the dependencies of poetry via $PYTHONPATH, making this package extremely broken in a nix-shell environment.
You would need to adopt a similar "vendoring" as the existing poetry package does not to have spectacularly broken imports.

@adisbladis
Copy link
Member

The use of poetry2nix within Nixpkgs is discouraged

By who exactly? By what decree?

@dotlambda
Copy link
Member Author

remove me as a maintainer from the package

Which package exactly? You're not listed as a maintainer of what is currently python3Packages.poetry.

IMHO this is the exact opposite direction to what nixpkgs should go towards as I see the current Python maintenance model in nixpkgs as unsustainable, and for the lack of a better word to express it, insane.

I think it's working pretty well actually, as proven by e.g. the fact that we can keep a package like Home Assistant that has more than a thousand direct dependencies working.
I also think it's much more sane security-wise than using a lock file for every package and not updating a dependency unless upstream does.
Finally, we run tests for most python3Packages which makes me much more confident about them actually working properly. Especially the many that require, on NixOS, patching in paths to non-Python dependencies.

The use of poetry2nix within Nixpkgs is discouraged

By who exactly? By what decree?

By the people who mostly maintain the Python packages. See #python:nixos.org.

@adisbladis
Copy link
Member

adisbladis commented Jan 12, 2023

I'm willing to concede on this even though I don't agree. I'm not especially interested in having this discussion time and time again..

Regardless the usability issue with $PYTHONPATH needs to be fixed before this is mergeable.

This was also done for poetry as packaged using poetry2nix.
@dotlambda
Copy link
Member Author

I tested poetry after removing $out/nix-support/propagated-build-inputs and it seems like symlinking poetry-core, as was done previously, isn't necessary. Can you check?

@adisbladis
Copy link
Member

I tested poetry after removing $out/nix-support/propagated-build-inputs and it seems like symlinking poetry-core, as was done previously, isn't necessary. Can you check?

That works.
Additionally a new version of Poetry was released yesterday. You might want to squeeze in the version bump in this PR too.

@dotlambda dotlambda mentioned this pull request Jan 12, 2023
13 tasks
@dotlambda dotlambda changed the base branch from staging-next to master January 14, 2023 19:49
@dotlambda dotlambda merged commit 2e1c2b9 into NixOS:master Jan 17, 2023
@dotlambda dotlambda deleted the poetry-no-poetry2nix branch January 17, 2023 07:26
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 17, 2023

IMHO this is the exact opposite direction to what nixpkgs should go towards as I see the current Python maintenance model in nixpkgs as unsustainable, and for the lack of a better word to express it, insane.

We cannot rely on upstreams lock files especially in the python world. The cryptography package is a perfect example why: Some upstreams pin it to before it was build with rust for various reasons but some of the more popular ones are that they either need to install rust, it takes to long to compile or because some cross platform issue. All those reasons do not apply to nixpkgs and are completely unacceptable to alone pin a 1.5 years old security relevant library for us.
cryptography is a direct dependency of hundreds of packages and a indirect one of thousands. Patching hundreds of lock files is unsustainable work.

Fully relying on upsteams lock files create security and maintainer nightmares like #141368


PS I also don't agree on the poetry2nix workflow, mainly that it is being vendored into nixpkgs. Most similar tools live fully outside nixpkgs and get patches applied in nixpkgs if necessary, hopefully only until the next update, or are fully inside the nixpkgs tree.

@K900 K900 mentioned this pull request Jan 17, 2023
13 tasks
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.

4 participants