-
-
Notifications
You must be signed in to change notification settings - Fork 451
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
build: remove bootstrap dependency on poetry #466
build: remove bootstrap dependency on poetry #466
Conversation
Here's a CI run where I'm building a musl-based image: https://github.com/cpcloud/protoletariat/runs/4576549106?check_suite_focus=true |
@adisbladis Thoughts on this PR? |
@@ -129,7 +129,6 @@ pythonPackages.callPackage | |||
++ lib.optional (stdenv.buildPlatform != stdenv.hostPlatform) pythonPackages.setuptools | |||
++ lib.optional (!isSource) (getManyLinuxDeps fileInfo.name).pkg | |||
++ lib.optional isDirectory buildSystemPkgs | |||
++ lib.optional (!__isBootstrap) pythonPackages.poetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break some downstreams (not all of the cases where this is required is covered by tests).
I would be open to something like !__isBootstrap && !isCross
so that we can solve your issue while not breaking various projects using this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write down such a scenario? I'm happy to write one or more test cases, so that folks don't spend a ton of time tracking down the problem to this (like I did) without a test breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really the same as any other build-system missing caused by python-poetry/poetry#2789.
Adding poetry
to each build was just a hack to circumvent that breakage for stuff depending on Poetry which I figured would be a fairly cheap workaround considering you are very likely to have poetry in your dependency graph anyway if you use this tool.
Maybe as a minimal reproduction case you could try an environment with https://github.com/tweag/pynixutil in it?
38d2b9b
to
22adc78
Compare
I've decided to merge this since the only downside is that we need a bit more overrides in the stdlib, but it's not possible to fix the cross use case via overrides. |
Since #466 was merged we don't need it anymore.
This PR removes a dependency on
poetry
that gets put into most dependenciesas a
buildInputs
if poetry2nix isn't bootstrapping, which if I'm reading thecode correctly is nearly everywhere (
__isBootstrap
isfalse
by default).This is a problem when building using
crossSystem
(I'm trying tobuild a Docker image using musl) because any dependency that overlapped with
poetry
's dependencies would cause both the build and host platform versionsof poetry's deps to be installed, causing a conflict during build.
It also appears that this bootstrap dependency is not necessary, at least
according to the current test suite. The only package that needed modification
was
pendulum
.