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

Fix PEP 660 metadata preparation fallback #10577

Merged
merged 7 commits into from
Oct 21, 2021

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Oct 12, 2021

When using pyproject.toml and the build backend does not support PEP 660, prepare metadata using build_metadata_for_build_wheel instead of setup.py egg_info. Fixes #10573.

Also fixes #10531, because this PR reworks and strengthen the detection of directories that are not python projects (i.e. they have neither a pyproject.toml nor a setup.py).

TODO

@sbidoul sbidoul added this to the 21.3.1 milestone Oct 12, 2021
@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch 2 times, most recently from 4fcbe5d to 1b34c0c Compare October 12, 2021 17:16
@sbidoul
Copy link
Member Author

sbidoul commented Oct 12, 2021

@pypa/pip-committers, @takluyver (as pep517 maintainer) I'd like to hear your opinion here.

Following my late realization that pip runs setup.py develop in an isolated environment when there is a pyproject.toml, I conclude that we need to make sure that the build environment is prepared with the correct dynamic build requirements before doing metadata preparation. [added] This means that using get_requires_for_build_editable is not correct when we later discover that the backend does not support PEP 660 and w fallback to setup.py develop.

Therefore we need to detect earlier if the build backend supports build_editable (PEP 660), i.e., before installing the build requirements, to decide if we call get_requires_for_build_wheel or get_requires_for_build_editable.

So I propose to add a mechanism to the pep517 library to query if the backend _supports_build_editable().

This actually nicely simplifies the frontend implementation, at the cost of one additional backend call to discover its capabilities.

Since there might be some urgency (#10573 being a regression), I propose to keep this mechanism private between pep517 (the library) and pip for now, until we eventually design a proper mechanism for pep517 to expose backend capabilities.

This is what I have done this in the second commit of this PR. Your comments are very much welcome.

@uranusjr
Copy link
Member

So I propose to add a mechanism to the pep517 library to query if the backend _supports_build_editable().

Should this be implemented as simply checking the existance of build_editable? I think this sounds reasonable as a stopgap; we can discuss the proper API after resolving the regression.

@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch from 1b34c0c to f0d0045 Compare October 13, 2021 06:19
@sbidoul
Copy link
Member Author

sbidoul commented Oct 13, 2021

So I propose to add a mechanism to the pep517 library to query if the backend _supports_build_editable().

Should this be implemented as simply checking the existance of build_editable? I think this sounds reasonable as a stopgap; we can discuss the proper API after resolving the regression.

Yep, I don't see an alternative, so I'm continuing in that direction.

@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch 5 times, most recently from 3b31f05 to 39db086 Compare October 13, 2021 13:23
@sbidoul
Copy link
Member Author

sbidoul commented Oct 13, 2021

@pypa/pip-committers this is not ready to merge (see TODOs above), but this is now in good shape and ready to review and raise any concern you might have with this (which I hope there aren't many :).

source_dir=self.unpacked_source_directory,
isolated=self.isolated,
details=self.name or f"from {self.link}",
)
Copy link
Member Author

Choose a reason for hiding this comment

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

At least prepare_metadata has become simpler.

@sbidoul sbidoul marked this pull request as ready for review October 13, 2021 14:31
@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch from 39db086 to f992465 Compare October 13, 2021 15:45
@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch 6 times, most recently from e3c47e7 to 9f94a31 Compare October 14, 2021 09:05
@sbidoul
Copy link
Member Author

sbidoul commented Oct 14, 2021

All done from my side, here. Now we need a pep517 release.

@pradyunsg
Copy link
Member

FYI: once the pep517 release is made, you'll want to rebase on main -- to get #10583 in this PR.

When there is a pyproject.toml, metadata preparation must be
done in the isolated build environment for legacy editable installs too
(fixes a regression).

Also detect earlier if an editable install must go through the
legacy install path, to be sure to run it in an environment
with the correct build requirements.
@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch from 9f94a31 to b07a956 Compare October 18, 2021 12:35
@sbidoul
Copy link
Member Author

sbidoul commented Oct 18, 2021

I vendored pep517 0.12 and rebased.

backend does not support :pep:`660`, prepare metadata using
``prepare_metadata_for_build_wheel`` instead of ``setup.py egg_info``. Also, refuse
installing projects that only have a ``setup.cfg`` and no ``setup.py`` nor
``pyproject.toml``. These restore the pre-21.3 behaviour.
Copy link
Member

Choose a reason for hiding this comment

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

Is the second sentence (the Also one) duplicating the 10531 fragment? I feel it should appeat only once (unfortunately Towncrier does not offer a natural way to reference two issues in one fragment).

Copy link
Member Author

Choose a reason for hiding this comment

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

It overlaps yes. This second one specifically covers editables and accepting it was a regression in 21.3 whereas 10531 is a fix of something introduced in 21.1 or 21.2.

@uranusjr
Copy link
Member

How does this change interact with non-pyproject.toml legacy setuptools editable installs (where we should still run setup.py egg_info without isolation)? I don't see special treatment of it here, does it go through another code path and is not relevant in this patch?

@sbidoul
Copy link
Member Author

sbidoul commented Oct 18, 2021

How does this change interact with non-pyproject.toml legacy setuptools editable installs (where we should still run setup.py egg_info without isolation)? I don't see special treatment of it here, does it go through another code path and is not relevant in this patch?

It should be covered in prepare_metadata (if use_pep517: ... else: prepare_metadata_legacy).

$ pip install -e setuppyonly/
Obtaining file:///home/me/tmp/setuppyonly
  Preparing metadata (setup.py) ... done
Installing collected packages: toto
  Running setup.py develop for toto
Successfully installed toto-0.1

vs

pip install -e pyproject+setupcfg/
Obtaining file:///home/me/tmp/pyproject%2Bsetupcfg
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
  Getting requirements to build wheel ... done
  Preparing wheel metadata (pyproject.toml) ... done
Installing collected packages: toto
  Running setup.py develop for toto
Successfully installed toto-0.0.0

@uranusjr
Copy link
Member

So mostly unrelated to the change made here. Thanks, that makes sense.

@sbidoul
Copy link
Member Author

sbidoul commented Oct 18, 2021

Which makes me think we should print Preparing metadata (pyproject.toml) instead of Preparing wheel metadata (pyproject.toml)..., except in PEP 660 mode where we print Preparing editable metadata (pyproject.toml)...

Do not mention wheel, as this code path is also used
in legacy editable mode with isolation, where
no wheel is involved.

This also aligns with the new log entry for setup.py egg-info
which says "Preparing metadata (setup.py)".
@sbidoul sbidoul force-pushed the fix-pep660-metadata-preparation-fallback branch from 928fec1 to 106042f Compare October 18, 2021 13:21
@uranusjr uranusjr merged commit d01dd4a into pypa:main Oct 21, 2021
@sbidoul sbidoul deleted the fix-pep660-metadata-preparation-fallback branch October 21, 2021 06:14
pradyunsg pushed a commit to pradyunsg/pip that referenced this pull request Oct 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants