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

Switch to PyPA build, installer, and flit-core to build Python packages #245509

Closed
wants to merge 23 commits into from

Conversation

tjni
Copy link
Contributor

@tjni tjni commented Jul 26, 2023

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, and installer 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:

  1. 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.

  2. 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.

  3. 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:

  1. Create python3.pkgs.bootstrap.{build,flit-core,installer} packages.
  2. Modify pypaBuildHook using build and introduce pypaInstallHook using installer.
  3. Swap pipBuildHook for pypaBuildHook and pipInstallHook for pypaInstallHook.
  4. Move tests for packages involved in bootstrapping to passthru to avoid cycles.
  5. Update wheel to a version that builds with flit-core to break its dependency cycle with setuptools.
  6. Clean up dependencies on boostrapped-pip.
  7. Update setuptools and pip to the latest versions. This turned out not to be strictly necessary, but I had already tested a lot of things with it before I realized this.

The reason for this long tail of commits is based on a few differences / design decisions:

  1. The old pipBuildHook adds pip and wheel to the PYTHONPATH. The new pypaBuildHook only includes build. 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 with setuptools implicitly needs wheel 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!

  2. 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.

  3. 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.

  4. 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

  1. 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.

  2. 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.

  3. Documentation. I haven't gotten around to doing that yet.

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.11 Release Notes (or backporting 23.05 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
  • Fits CONTRIBUTING.md.

@K900
Copy link
Contributor

K900 commented Jul 26, 2023

We shouldn't need a bootstrap pip/setuptools at all, we can build them as normal with this. Also, appreciate the work!

@K900 K900 requested review from mweinelt, dotlambda and FRidh and removed request for mweinelt July 26, 2023 09:05
@FRidh
Copy link
Member

FRidh commented Jul 26, 2023

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.

  1. Rebuild flit-core using bootstrapped build and installer.

  2. Rebuild build using flit-core, bootstrapped build, and bootstrapped installer.

  3. Rebuild installer using flit-core, build, and bootstrapped installer.

From my point of view we can have a bootstrapped-build and bootstrapped-installer that are then used in the hooks. The bootstrapped-* could be build by just installing all its deps in the same store path. Dumb and simple. Of course splitting things up further is fine but I think it should be optional.

  1. Switch to pypaBuildHook when pyproject is the format.

  2. Introduce and switch to pypaInstallerHook when pyproject is the format.

  3. Deprecate format = flit (?)

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 pyproject was introduced.

  1. Clean up bootstrapped-pip.

We should be able to remove it.

8. Clean up `setuptools` bootstrapping.

@K900
Copy link
Contributor

K900 commented Jul 26, 2023

I think we should really have an extra step in the bootstrap to build build and installer properly, as they're supposed to also be used as modules by other tools (e.g. poetry) so we would need them as clean packages anyway.

@K900
Copy link
Contributor

K900 commented Jul 27, 2023

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)

@tjni tjni force-pushed the bootstrap-python branch 2 times, most recently from 1421a0e to 439c4fe Compare July 27, 2023 07:05
@tjni tjni marked this pull request as draft July 28, 2023 07:24
@tjni tjni changed the base branch from master to staging August 1, 2023 09:17
@tjni
Copy link
Contributor Author

tjni commented Aug 11, 2023

Helpfully, the build for asn2quickder.x86_64-linux yielded an error:

Executing pypaInstallPhase
usage: python -m installer
       [-h]
       [--destdir path]
       [--prefix path]
       [--compile-bytecode level]
       [--no-compile-bytecode]
       wheel
python -m installer: error: unrecognized arguments: dist/arpa2.quickder_tools-1.7.1-py3-none-any.whl

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.

@FRidh
Copy link
Member

FRidh commented Aug 12, 2023

Upstream's recommended "python -m build" way of invoking build fails
when the working directory contains a file named "build.py". This is
common for poetry projects that build C extensions.

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 python-updates branch.

@FRidh
Copy link
Member

FRidh commented Aug 12, 2023

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.

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.

@tjni
Copy link
Contributor Author

tjni commented Aug 12, 2023

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.

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.

Please avoid force pushing. I took your commits and pushed them to the python-updates branch.

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.

It's better to install them simultaneously in a single invocation.

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.

@FRidh
Copy link
Member

FRidh commented Aug 12, 2023

I just started a new evaluation on hydra with this change rebased onto staging. https://hydra.nixos.org/jobset/nixpkgs/python-updates

Copy link
Contributor

@doronbehar doronbehar left a 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.

@tjni
Copy link
Contributor Author

tjni commented Aug 12, 2023

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.

@FRidh
Copy link
Member

FRidh commented Aug 13, 2023

I just started a new evaluation on hydra with this change rebased onto staging. https://hydra.nixos.org/jobset/nixpkgs/python-updates

Hydra seems to be occupied. Looks like we need to do it without it.

@tjni
Copy link
Contributor Author

tjni commented Aug 13, 2023

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).

@FRidh FRidh mentioned this pull request Aug 13, 2023
12 tasks
@FRidh
Copy link
Member

FRidh commented Aug 13, 2023

@tjni could you add further changes to #248866.

@tjni
Copy link
Contributor Author

tjni commented Aug 13, 2023

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.

@doronbehar
Copy link
Contributor

This can be closed right?

@tjni
Copy link
Contributor Author

tjni commented Aug 18, 2023

Closing in favor of #248866.

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.

None yet

6 participants