Skip to content

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Sep 28, 2025

closes #6334
closes #11457
closes #11859

This is the big one - it does not make much sense to split in smaller PRs.

  • --use-pep517 always on
  • --no-use-pep517 removed (error if used)
  • --global-option and --build-option removed (error if used)
  • legacy editable (setup.py develop) removed
  • legacy build (setup.py bdist_wheel) removed
  • legacy metadata preparation (setup.py egg_info) removed

There is still a lot to do in the test suite, but it looks like it will mostly be adding --no-build-isolation to many tests, in order to use the locally installed setuptools. I'm opening as draft nevertheless in case there are any suggestions.

I tried to split in small commits that should be relatively easy to review.

Documentation update will follow.

@sbidoul sbidoul force-pushed the pep517-only-sbi branch 4 times, most recently from 794a9eb to b9733f3 Compare September 28, 2025 15:00
@sbidoul sbidoul marked this pull request as ready for review September 28, 2025 16:11
We remove
- legacy metadata preparation
- legacy wheel build (bdist_wheel)
- --global-option and --build-option
- --no-use-pep517
- and therefore all setuptools shims

--use-pep517 stays as a no-op

This commits removes some tests that become irrelevant,
but just the minimum so mypy is happy.
This option is now always on, so need to test it.
This option is gone.
Some unit test need to run metadata preparation for setuptools based test projects.
Because pip now uses flit_core as build backend.
Most tests use setup.py-based test packages, and we
want to avoid network access for building them.
@sbidoul sbidoul added this to the 25.3 milestone Sep 28, 2025
@ichard26
Copy link
Member

In terms of availability, I should have some (although not a lot!) time to review starting in mid-late October. If you want to wait for reviews (since this is a major change), then you may want to take that into consideration. Otherwise, it will depend on whether Paul, Pradyun or Damian have any time for reviews.

Anyway, thank you a lot for working on this!

Since there is no 'setup.py develop' fallback anymore,
this check needs to be updated, and also needs to run
when not using build isolation.
@notatallshaw
Copy link
Member

I will try and add my review before 25.3, although this is really outside my wheelhouse (pun intended), so I'll only really be reviewing code quality and doing some smoke tests, I assume there aren't many design choices here, but if there are I won't be able to add much useful input.

@sbidoul
Copy link
Member Author

sbidoul commented Sep 29, 2025

Thanks @ichard26 and @notatallshaw ! I'll also try to make some time to review other 25.3 PRs in the coming 2 weeks.

Regarding this PR, there were not any design decisions left to do. So it's a big diff but it's straightforward removal of all non-pep517 code paths. The code base was already prepared for that thanks to the work we progressively did over the last (10?) years - kudos to everyone involved, by the way!

The vast majority of the changes to the test suite is adding --no-build-isolation in many places so tests don't have to look for setuptools. A few other tests had to be removed as they were testing removed features, or needed to be adjusted to cope with removed options (such as using --config-setting instead of --global-option).

Copy link
Member

@notatallshaw notatallshaw left a comment

Choose a reason for hiding this comment

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

I've done a light reading, tried to find edge case issues, and I've run a bunch of smoke tests, and it all looks good to me.

@sbidoul
Copy link
Member Author

sbidoul commented Oct 14, 2025

Thanks for this @notatallshaw. I still need to update the documentation.

@ichard26
Copy link
Member

@sbidoul I am slowly reviewing this PR. I expect to be done in a few days. A few notes though:

  • I don't think the InstallRequirement.isolated attribute is used anymore. It was only used by the legacy setuptools flows. Unless there is a possible future use for that attribute (maybe process isolation during build?), I'd prefer if we got rid of it. It's being passed around everywhere for no real gain.

  • I'm not the biggest fan of adding --no-build-isolation everywhere since it does deviate from the normal installation flow. I'd prefer that we used a variation of pip_install_local() that passed --no-index1 and --find-links pointing to the common wheels cache. However, doing that would be substantially slower since build isolation still has a major performance penalty (while Install build dependencies in-process #13450 will help a lot, just installing setuptools can be slow). Due to that, I agree it's better to use --no-build-isolation for now.

Footnotes

  1. A long-term goal of mine is clean up the test suite to avoid using the network as much as possible. Using a helper that uses --no-index would go a long way here.

@sbidoul
Copy link
Member Author

sbidoul commented Oct 14, 2025

@ichard26 thanks!

I don't think the InstallRequirement.isolated attribute is used anymore

Did this PR do that? I don't immediately see it. If not I'd rather do that in another PR.

I'm not the biggest fan of adding --no-build-isolation everywhere [in the test suite]

Me neither. I had the same reasoning as you, plus I did not want to mix a huge refactor of the test suite in this PR so it remains feasible to review that the tests did not change what they intended to test.

@ichard26
Copy link
Member

Did this PR do that? I don't immediately see it. If not I'd rather do that in another PR.

@sbidoul, actually I double checked and it appears that InstallRequirement.isolated also influences the installation scheme when the distutils locations backend is used ( 😔). So I guess we can only remove the attribute when our distutils code is dead. (@uranusjr do you know what the timeline looks like for that?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants