-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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
Switch to PyPA build, installer, and flit-core to build Python packages #245509
Conversation
We shouldn't need a bootstrap pip/setuptools at all, we can build them as normal with this. Also, appreciate the work! |
Thanks for looking into this. It's badly needed and if am correct even blocking us on I think updating pip otherwise. These are indeed the right steps. Comments inline.
From my point of view we can have a
This can be done independently from this work. In my opinion it is OK to not even deprecate but outright remove the functionality. It was a temporary solution that was needed many years back. Nobody should depend on this since
We should be able to remove it.
|
I think we should really have an extra step in the bootstrap to build |
FWIW this entire thing will end up in staging anyway as soon as you touch any of the hooks and/or pip and/or setuptools, so I wouldn't worry about it too much. (re: last commit message) |
In this new version of wheel, the build backend changed from setuptools to flit-core. This is nice because it breaks a cycle between setuptools and wheel.
Co-authored-by: adisbladis <[email protected]>
We need to clarify these or else the wrong build system gets added and we find ourselves with an infinite recursion.
This version update is needed to be compatible with pip 23.2.
Helpfully, the build for asn2quickder.x86_64-linux yielded an error:
This was because we did not handle installing when a build creates more than one wheel. I updated pypaInstallHook to do this properly now in a loop. |
Has this been reported upstream (PyPA/build)? It's inevitable that name clashes can occur, but by choosing a different name it does get less likely. I am testing this now in hydra as well https://hydra.nixos.org/jobset/nixpkgs/python-updates. Please avoid force pushing. I took your commits and pushed them to the |
It's better to install them simultaneously in a single invocation. Python dependencies can be circular, they do not need to be on a DAG. Hence, it can be possible the only way to install packages is to install them together. |
The closest issue I could find to this is pypa/build#526, in which a contributor acknowledges that the naming is unfortunate. I think it's too late to change, and luckily (or by design) they ship a CLI named pyproject-build that we can use instead.
Thank you for kicking off the test run, I've been using its results today. I'll make sure any changes I make are new commits.
I didn't see a way in the documentation of installer to do this or an issue in their repository when I did a quick search. Does installation need to happen in a particular order? I was under the impression that this step doesn't do any dependency checking. |
I just started a new evaluation on hydra with this change rebased onto staging. https://hydra.nixos.org/jobset/nixpkgs/python-updates |
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.
Also, ofborg evaluation fails currently.
Thanks for bringing it to my attention. I'm avoiding rebasing right now to not mess up the testing on python-updates that @FRidh is helping me with, but it will be resolved prior to merging. |
Hydra seems to be occupied. Looks like we need to do it without it. |
That's ok, still plenty to do, and the last run yielded some good information. When the staging-next queue clears up, we might be able to get a few more builds in (with hopefully some more fixes too). |
Ah, I can but I think I need to get write permissions first on the repo to push to that PR. I'll send in a request tomorrow. |
This can be closed right? |
Closing in favor of #248866. |
I have updated the issue description with the latest status.
Background
We are going to adopt
build
as our bootstrap build front-end,flit-core
as our bootstrap build backend, andinstaller
as our bootstrap wheel installer.This is based heavily on this gist that @K900 shared with me. The only difference is that
build
is not used to bootstrap itself, which simplifies the build a little.I want to acknowledge prior work from @FRidh in #105714 and @mweinelt in #232451, which introduced me to this idea.
TLDR
There are a lot of commits in this PR – continue reading to find out why. I think this is a good approach in the long term, but I expect some breakages will be teased out on Hydra or afterward. I am confident that all packages can be fixed quickly once we've noticed a breakage.
How do I review this monstrosity?
Thank you in advance!!! The description below will explain in more detail the changes, but I have split up the commits into the following sections:
The most important commits to review come first. Their descriptions should explain what they do. This includes the implementation of the bootstrapping, hooks, and key package updates. These are worth reviewing carefully.
What follows that is a series of commits that fix up packages to add build dependencies, patch version constraints, fix tests, etc. Each is small and self-explanatory. I suggest sampling some that seem interesting. I tried to use similar commit messages for similar changes, and they are ordered alphabetically by attribute path.
The final set of commits all have the commit message "add wheel dependency", and (unless I messed up by accident) they only add
wheel
to a package's nativeBuildInputs, because it is no longer being propagated automatically. I don't recommend reading these unless you really want to, but they are also ordered alphabetically by attribute path.Description of changes
This PR starts with a few commits updating maintainers, then about a dozen and a half of core commits, followed by a long, long tail of commits fixing things found based on the approach I am taking.
The core commits roughly do the following:
The reason for this long tail of commits is based on a few differences / design decisions:
The old
pipBuildHook
addspip
andwheel
to the PYTHONPATH. The newpypaBuildHook
only includesbuild
. This is a design decision: I think introducing as little into the environment as possible simplifies understanding how builds work. The consequence, however, is that any package that builds withsetuptools
implicitly needswheel
as defined in its PEP 517 get_requires_for_build_wheel hook. When this is missing, build will fast-fail with an error message. I've done my best to identify the couple hundred of packages that need this added to its nativeBuildInputs, but I expect some will slip by and require fixing after the fact. Sorry!Another consequence of this change is that
pip
is no longer part of the default PYTHONPATH, and so some packages need to add it explicitly in their tests.build
does both pyproject.toml validation and stricter dependency checking by default. This means some packages will need patching to its pyproject.toml or additional dependencies added to its nativeBuildInputs. I have also tried to identify as many of these as I could ahead of time.The new version of setuptools issues a deprecation warning when
pkg_resources
is imported. Some projects turn all warnings into errors during tests and so will need to be patched to ignore the warning.Next steps
Appreciate any feedback on the approach. Even though there are a lot of commits here already, I don't care about sunk cost and don't mind backing out anything, doing it in smaller chunks, or anything else.
If this approach seems reasonable, then any feedback or help with testing is welcome. Note that these changes seem to be used in the build path of LLVM, so you will need to build some stuff if testing locally even if testing on a branch with Hydra artifacts. I will commit to keeping this PR up to date with staging when merge conflicts are discovered.
Documentation. I haven't gotten around to doing that yet.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)