build: implement metadata preparation hook

[uranusjr]

Fix #130.

I make the function signature require the user to pass in "wheel" explicitly. This is of course redundant, but I have a feeling we’re going to have a prepare_metadata_for_sdist at some point and it’s better to leave room now than having to change things later. And prepare("wheel") is arguably more consistent with build("wheel") anyway.

Due to the issues mentioned in #130 (comment), I’m not using the build_wheel fallback provided by pep517, and instead return None when the backend does not implement prepare_metadata_for_wheel. The returned result is stored in a member attribute and popped out when build_wheel is called.

[layday]

I like that prepare follows the same pattern as build. Why is the metadata path discarded during the build?

[uranusjr]

PEP 517 does not say whether the prepared metadata directory may be reused across multiple build_wheel call, only that it should be reused, so I took the safe route and use it only exactly once.

It also just occured to me that the PEP says the frontend should only pass the metadata directory into build_whell [if the frontend] depends on the wheel resulting from this call to have metadata matching this earlier call, so an option should be provided for the frontend to have a choice to not do it. So maybe it’s not that good an idea to store the prepare result as a state after all, and we should just add a metadata_directory argument to build and let the user choose what to they want.

[layday]

Yes, I don't think we need to do anything fancy with the metadata path, nor do I expect that anybody will actually want to pipe the metadata path into build_wheel. I'm not sure if it was intended as an optimisation should you need to look at the metadata before building the wheel or a guarantee that the source tree has not changed between the two calls, but nobody seems to have sought to use the metadata_directory argument of build_wheel, that I can tell. That might be because none of the three major backends support it:

• https://github.com/pypa/setuptools/blob/v52.0.0/setuptools/build_meta.py#L214-L217
• https://github.com/takluyver/flit/blob/3.0.0/flit_core/flit_core/buildapi.py#L59-L62
• https://github.com/python-poetry/poetry-core/blob/1.0.0/poetry/core/masonry/api.py#L53-L57

[uranusjr]

nobody seems to have sought to use the metadata_directory argument of build_wheel, that I can tell.

pip does. And arguably all major backends are implemented incorrectly. According to PEP 517, if a backend implements prepare_metadata_for_wheel, it MUST ensure the resulting wheel contains identical metadata to the prepared metadata directory (IOW it must use the metadata_directory argument to either generate the wheel directly from it, or use it to verify the built wheel contains identical metadata). setuptools actually fails to meet the promise in at least one situation, leading to numerous pip bug reports.

[layday]

setuptools actually fails to meet the promise in at least one situation, leading to numerous pip bug reports.

Is that because the prepared metadata becomes stale (e.g. when editing a package locally) or because the prepared metadata is simply incorrect?

[uranusjr]

The prepared metadata is incorrect in the particular case (pypa/pip#9446).

[layday]

We could check with backend devs to see why this wasn't implemented, if the PEP needs to be modified and if pip wouldn't be better off comparing prepared and built wheel metadata on their end.

[uranusjr]

There are issue filed to those backends now (I filed one for Flit just a moment ago): pypa/setuptools#1825 python-poetry/poetry#1078 pypa/flit#389

[uranusjr]

I raised the issue on discussion.python.org, and from the response, I think we should still implement the metadata_directory argument because it is the right thing to do. Backends don’t use it, but that’s an issue they should fix.

[FFY00]

I agree. I think a backend simply ignoring the argument is fine, though I think they should all verify if it's the same metadata if they're gonna rebuild it anyway.

[uranusjr]

prepare('wheel') currently returns None if prepare_metadata_for_build_wheel is not implemented. Should an exception be raised instead?

[layday]

Possibly - would that mean exposing pep517's exceptions or creating our own equivalents?

[layday]

That is to say, I don't think a generic Build(Backend)Exception is satisfactory.

[uranusjr]

Agreed. The exception raised here need to be different, otherwise the caller won’t be able to distinguish an expected code path (prepare_metadata_for_build_wheel not implemented by the backend) from a legistimate error in the backend (BuildBackendException) or the build process (BuildException). So a new exception will be needed if we raise here.

[layday]

metadata_directory -> metadatadir or outdir -> output_directory for consistency.

[uranusjr]

PEP 517 names the arguments wheel_directory and metadata_directory. Would that work?

[layday]

Well, build works both with wheels and sdists, hence 'out' or 'output'.

[uranusjr]

I’m going with output_directory.

[gaborbernat]

@uranusjr made some changes to reduce code duplication, but now looks 👍🏻