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.

Sign up for GitHub

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

Jump to bottom

Improve core metadata compliance for PKG-INFO #3904

Merged
merged 10 commits into from
Aug 29, 2023
Merged

Improve core metadata compliance for PKG-INFO #3904

merged 10 commits into from
Aug 29, 2023

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Apr 24, 2023 •
edited
Loading

Summary of changes

  • Add Requires-Dist to PKG-INFO files and make them compatible with .dist-info/METADATA
  • This PR extracts PKG-INFO logic from setuptools.dist to setuptools._core_metadata.
  • Improve 'atomicity' when writing .egg-info/PKG-INFO.
    • It seems that there is a race condition between importlib.metadata reading PKG-INFO and setuptools.build_meta writing to PKG-INFO, which causes importlib.metadata to emit a deprecation warnings (which will be converted in the future into an error).
    • I have observed in the past errors in the CI behaving in a flaky way, and I think those were associated to this race condition.

In the future the _core_metadata module can be used directly to generated .dist-info/METADATA.

This is part of a series of PRs:

The motivation for this series of PRs is the following:

Pull Request Checklist

This was referenced Apr 24, 2023
Merged
Closed
Closed
Closed
Closed
@abravalheri abravalheri force-pushed the dev/core_metadata branch 5 times, most recently from 5684826 to 07914b3 Compare April 26, 2023 17:01
@abravalheri
Copy link
Contributor Author

After some investigation and debugging it seems that the PKG-INFO file starts with the correct content, but ends up empty when the tests start running.

There might be a test that is not isolated modifying the PKG-INFO file directly...

This seems to be a flaky behaviour, and I am having trouble to replicate it locally, but I can see it in can some tests in the CI.

@abravalheri

This comment was marked as outdated.

Sign in to view
@abravalheri
Copy link
Contributor Author

I noticed that when I improve the isolation of egg_info1 used in setuptools.build_meta:get_requires*2, setuptools starts to get trouble to bootstrap.
It may seem that it is relying on the fact that setuptools.build_meta:get_requires* will generate a setuptools.egg-info/PKG-INFO file in the project hook that will be picked up by importlib.metadata in the subsequent hook calls.

Footnotes

  1. by setting egg_base to a temporary directory ↩

  2. The isolation is necessary to avoid race conditions caused by importlib.metadata reading stuff from the .egg-info folder in the project root, while the .egg-info folder is being written. This situation seems to occur frequently because we use pytest-xdist, pytest-checkdocs, pytest-perf and other plugins that consume project metadata using build.util.project_wheel_metadata. I have been observing flaky tests because of this behaviour in the past months. ↩

@abravalheri

This comment was marked as outdated.

Sign in to view
@abravalheri
Copy link
Contributor Author

If we try to further enhance egg_info isolation to avoid race conditions in the tests, and set up egg_base when packaging setuptools itself, the setuptools sdist will not contain the setuptools.egg-info folder... This means that it will not have entry_points.txt.

However some attributes in setup.cfg and setup.py are implemented via the distutils.setup_keyworks entry-point. Which means that they will not be available for being used and setting these attributes will cause errors.

@abravalheri
Copy link
Contributor Author

It seems that an easier approach is not try to change where egg_info writes files for now (since this task is very difficult). Instead I tried to make write_pkg_info a more atomic operation.

@abravalheri abravalheri marked this pull request as ready for review May 10, 2023 13:51
@abravalheri abravalheri mentioned this pull request May 11, 2023
Open
@abravalheri
Copy link
Contributor Author

@jaraco, would you be OK if I move this PR (#3904) and #3903 ahead?1

The main idea of the 2 PR's is to improve PKG-INFO (right now install_requires and extras_require are mainly analysed by setuptools from a *.egg-info/requires.txt perspective, these 2 PRs shift the perspective to concern more with PKG-INFO).

Footnotes

  1. I know that I presented them as part of a bigger series, but for now I just would like merging these 2 PRs. The rest still depends on discussions with pypa/wheel and pypa/packaging. ↩

@jaraco
Copy link
Member

jaraco commented Aug 22, 2023

No problem. Please proceed. I trust your instincts.

@abravalheri
Record normalized requirements in dist.metadata object
a641c78
@abravalheri
Use special attributes with requirements for PKG-INFO
40d930f
@abravalheri
Adjust apply_pyproject tests
2239b8a
@abravalheri
Re-use pre-parsed requirements when writing requires.txt
3c9d6ac
@abravalheri
Test parity between setuptools PKG-INFO and wheel METADATA
13d1e9f
@abravalheri
Improve atomicity when writing PKG-INFO
f4dd7e2 
For the time being, when `setuptools.build_meta` is called,
`egg_info.egg_base` is accidentally set to the project root between the
several calls to the different build hooks.

This means that if the hooks are called, they will try to overwrite
`setuptools.egg-info/PKG-INFO`, and for a very short interval of time it
will be an empty file.

Another process may then try to simultaneously use `importlib.metadata`
to list entry-points. However to sort entry-points, `importlib.metadata`
will try to read the `Name` field in the `PKG-INFO/METADATA` files and
will raise an error/warning if that file is empty.

This commit tries to use `os.replace` to avoid having an empty PKG-INFO
while `importlib.metadata` is used.
@abravalheri abravalheri mentioned this pull request Aug 29, 2023
Merged
2 tasks
Base automatically changed from dev/egg_info_requires to main August 29, 2023 17:44
@abravalheri abravalheri merged commit b537f53 into main Aug 29, 2023
58 checks passed
@abravalheri abravalheri deleted the dev/core_metadata branch August 29, 2023 17:44
@uranusjr uranusjr mentioned this pull request Sep 7, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abravalheri @jaraco