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

build meta wheel does not respect metadata_directory per PEP-517 #1825

Closed
gaborbernat opened this issue Aug 19, 2019 · 12 comments · Fixed by #4647
Closed

build meta wheel does not respect metadata_directory per PEP-517 #1825

gaborbernat opened this issue Aug 19, 2019 · 12 comments · Fixed by #4647

Comments

@gaborbernat
Copy link
Contributor

gaborbernat commented Aug 19, 2019

From https://www.python.org/dev/peps/pep-0517/#build-wheel:

If the build frontend has previously called prepare_metadata_for_build_wheel and depends on the wheel resulting from this call to have metadata matching this earlier call, then it should provide the path to the created .dist-info directory as the metadata_directory argument.

https://github.com/pypa/setuptools/blob/master/setuptools/build_meta.py#L208 though ignores the passed in metadata_directory argument and goes to regenerate this information in the root of the project.

    def build_wheel(self, wheel_directory, config_settings=None,
                    metadata_directory=None):
        return self._build_with_temp_dir(['bdist_wheel'], '.whl',
                                         wheel_directory, config_settings)
@dholth
Copy link
Member

dholth commented Dec 23, 2020

IMO the passed directory is not necessary. It will certainly match the recently generated one.

@gaborbernat
Copy link
Contributor Author

IMO the passed directory is not necessary. It will certainly match the recently generated one.

First and foremost it's mandated per PEP-517, so you MUST conform to that expectation.

Second, you can't really make the assumption that it will match. For example, the user might have invoked manually (aka via python setup.py, and then the metadata directory defaults to the default location (in-place of the source tree at the moment); while frontends may want to move the metadata directory out of the source tree into a frontend specific folder (such as .tox). So I'm not sure how you can make that certainly assumption, without causing headaches and subtle bugs.

@dholth
Copy link
Member

dholth commented Dec 24, 2020

I think the current backends always rebuild dist-info as part of the wheel build. The newly rebuilt dist-info is very likely to match the one built moments earlier by the other hook

@gaborbernat
Copy link
Contributor Author

The newly rebuilt dist-info is very likely to match the one built moments earlier by the other hook

I'm not sure I follow how this is related to the fact that this hook should take and use the metadata_directory, maybe the frontend wants to use the content of the folder afterward, not?

@pradyunsg
Copy link
Member

Updated OP's post to a permalink:

def build_wheel(self, wheel_directory, config_settings=None,

@uranusjr
Copy link
Member

This is also discussed in pypa/build#217. Although setuptools is free to rebuild dist-info in build_wheel (instead of using the content of metadata_directory), it should at least verify the built wheel indeed contains identical metadata to those provided in metadata_directory. #2529 is a concrete example of setuptools failing expectations due to this nonconformance.

This has also been raised to flit (pypa/flit#389) and poetry (python-poetry/poetry#1078).

@dholth
Copy link
Member

dholth commented Feb 1, 2021 via email

@uranusjr
Copy link
Member

uranusjr commented Feb 1, 2021

But then what happens if a backend return different things from build_wheel and prepare_metadata_for_build_wheel? (Which setuptools is guity of doing right now.) It is certainly possible to remove the requirement for backend to make use of the metadata_directory argument, but the backend should still somehow return consistent things from the two hooks. Otherwise the frontend would be stuck and can’t proceed. The discussion needs to be moved to discuss.python.org if you want to do that so frontend authors can join the conversation.

@wimglenn
Copy link
Contributor

wimglenn commented May 15, 2022

setuptools-ext is a thin wrapper around setuptools.build_meta allowing to specify some more metadata fields such as Requires-External, which aren't otherwise supported by setuptools. Passing around the metadata_directory (ref) would have been really useful if it worked!

If setuptools were to diligently follow PEP 517 (i.e. does not just ignore the metadata_directory), we could simply call prepare_metadata_for_build_wheel before dropping in some additional files, or modify existing ones, under .dist-info subdir, and then call build_wheel passing in the metadata dir.

However since that didn't work, instead setuptools-ext has to call build_wheel, and then completely rewrite the result, which is a lot more complicated (considering that modifying the METADATA file also invalidates the RECORD file!)

This "tweaking" of a build backend by wrapping the hooks is apparently an expected usage, because it's documented as a way for an in-tree build backend to augment the build dependencies (by @abravalheri here). It's unfortunate that one can't reliably use a similar technique for prepare_metadata_for_build_wheel + build_wheel.

@pelson
Copy link
Contributor

pelson commented Apr 23, 2024

I just encountered exactly the same thing as seen by setuptools-ext - modifying the metadata in prepare_metadata_for_build_wheel would be super useful (and PEP-517 spec compliant).

I'm guessing it isn't already easy to build using an existing dist_info directory with setuptools, hence the reason for not having implemented this already?

@abravalheri
Copy link
Contributor

abravalheri commented Apr 23, 2024

I'm guessing it isn't already easy to build using an existing dist_info directory with setuptools, hence the reason for not having implemented this already?

Precisely. Things are complicated because of the division between the setuptools and wheel packages.

Right now the METADATA file should be similar to the PKG-INFO considering that there is no other packaging wrapping setuptools. So setuptools/wheel kind of try1 to comply with the PEP requirements2 as long as there is no other "man-in-the-middle" modifying the files. I believe other backends also adopt the approach of re-generating the file.

Footnotes

  1. Tries but probably fails in some edge cases, because it is a difficult problem.

  2. The PEP does not necessarily excludes the possibility of re-generating the files, but it requires them to a "non-modified" version of each other...

@pelson
Copy link
Contributor

pelson commented Apr 24, 2024

Curiosity got the better of me, and I opened an issue in pypa/wheel#611 and prototyped in pypa/wheel#612. With proposed PR to wheel, and the following change in setuptools, build_wheel would be PEP-517 compliant on this topic (taking the approach of using the dist-info metadata, rather than re-computing and then subsequently validating):

diff --git a/setuptools/build_meta.py b/setuptools/build_meta.py
index 2decd2d21..58b50cfc4 100644
--- a/setuptools/build_meta.py
+++ b/setuptools/build_meta.py
@@ -406,9 +406,12 @@ class _BuildMetaBackend(_ConfigSettingsTranslator):
     def build_wheel(
         self, wheel_directory, config_settings=None, metadata_directory=None
     ):
+        cmd = ['bdist_wheel']
+        if metadata_directory:
+            cmd.extend(['--dist-info-dir', metadata_directory])
         with suppress_known_deprecation():
             return self._build_with_temp_dir(
-                ['bdist_wheel'],
+                cmd,
                 '.whl',
                 wheel_directory,
                 config_settings,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants