Skip to content

PEP 517: remove prepare_wheel_metadata references #311

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

Merged
merged 2 commits into from
Jul 20, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 42 additions & 37 deletions pep-0517.txt
Original file line number Diff line number Diff line change
Expand Up @@ -181,18 +181,18 @@ Must build a .whl file, and place it in the specified ``wheel_directory``. It
must return the basename (not the full path) of the ``.whl`` file it creates,
as a unicode string.

If the build frontend has previously called ``prepare_wheel_metadata`` and
depends on the wheel resulting from this call to have metadata
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. If this
argument is provided, then ``build_wheel`` MUST produce a wheel with identical
metadata. The directory passed in by the build frontend MUST be
identical to the directory created by ``prepare_wheel_metadata``,
identical to the directory created by ``prepare_metadata_for_build_wheel``,
including any unrecognized files it created.

Backends which do not provide the ``prepare_wheel_metadata`` hook may either
silently ignore the ``metadata_directory`` parameter to ``build_wheel``, or
else raise an exception when it is set to anything other than ``None``.
Backends which do not provide the ``prepare_metadata_for_build_wheel`` hook may
either silently ignore the ``metadata_directory`` parameter to ``build_wheel``,
or else raise an exception when it is set to anything other than ``None``.

If ``build_directory`` is not None, it is a unicode string containing the
path to a directory other than the source directory (the working directory where
Expand Down Expand Up @@ -744,12 +744,13 @@ across the ecosystem.
more powerful options for evolving this specification in the future.

For concreteness, imagine that next year we add a new
``prepare_wheel_metadata2`` hook, which replaces the current
``prepare_wheel_metadata`` hook with something that produces more data, or a
different metadata format. In order to
manage the transition, we want it to be possible for build frontends
to transparently use ``prepare_wheel_metadata2`` when available and fall
back onto ``prepare_wheel_metadata`` otherwise; and we want it to be
``build_sdist_from_vcs`` hook, which provides an alternative to the current
``build_sdist`` hook where the frontend is responsible for passing
version control tracking metadata to backends (including indicating when all
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be more realistic than the example it replaces, but I think that trying to understand it is more likely to distract the reader from the point we're making here about why a Python API is nicer than a command line API. The more abstract 'different metadata format' seems clearer to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but I don't even want to put the suggestion out there that we might be thinking about changing the wheel metadata format.

Honestly, given the rejection of PEP 516, it might make sense to cull this whole section severely, as it's mainly a distraction know.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to cutting this comparison section down to a brief paragraph, or moving it to an appendix or something (can PEPs have appendices?). It's a large amount of text which doesn't really help to describe what the PEP is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, PEPs can definitely have appendices - they still appear in the same document, they're just moved to the end where they're more clearly out of the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll note that's a bigger structural change than I'm prepared to draft as BDFL-Delegate, though - it's subjective enough that I think it's best handled directly by the PEP authors, rather than treating it as a reviewer proposed fix or clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'll have a go at that once your new PR is merged.

on disk files are tracked), rather than individual backends having to query that
information themselves. In order to manage the transition, we'd want it to be
possible for build frontends to transparently use ``build_sdist_from_vcs`` when
available and fall back onto ``build_sdist`` otherwise; and we'd want it to be
possible for build backends to define both methods, for compatibility
with both old and new build frontends.

Expand All @@ -767,11 +768,11 @@ achieve. Because ``pip`` controls the code that runs inside the child
process, it can easily write it to do something like::

command, backend, args = parse_command_line_args(...)
if command == "prepare_wheel_metadata":
if hasattr(backend, "prepare_wheel_metadata2"):
backend.prepare_wheel_metadata2(...)
elif hasattr(backend, "prepare_wheel_metadata"):
backend.prepare_wheel_metadata(...)
if command == "build_sdist":
if hasattr(backend, "build_sdist_from_vcs"):
backend.build_sdist_from_vcs(...)
elif hasattr(backend, "build_sdist"):
backend.build_sdist(...)
else:
# error handling

Expand All @@ -786,28 +787,32 @@ any change can go live, and that any changes will necessarily be
restricted to new releases.

One specific consequence of this is that in this PEP, we're able to
make the ``prepare_wheel_metadata`` command optional. In our design, this
can easily be worked around by a tool like ``pip``, which can put code
in its subprocess runner like::

def prepare_wheel_metadata(output_dir, config_settings):
if hasattr(backend, "prepare_wheel_metadata"):
backend.prepare_wheel_metadata(output_dir, config_settings)
else:
backend.build_wheel(output_dir, config_settings)
touch(output_dir / "PIP_ALREADY_BUILT_WHEELS")
unzip_metadata(output_dir/*.whl)

def build_wheel(output_dir, config_settings, metadata_dir):
if os.path.exists(metadata_dir.parent / "PIP_ALREADY_BUILT_WHEELS"):
copy(metadata_dir.parent / *.whl, output_dir)
else:
backend.build_wheel(output_dir, config_settings, metadata_dir)

and thus expose a totally uniform interface to the rest of ``pip``,
make the ``prepare_metadata_for_build_wheel`` command optional. In our design,
this can be readily handled by build frontends, which can put code in
their subprocess runner like::

def dump_wheel_metadata(backend, output_dir, config_settings):
if hasattr(backend, "prepare_metadata_for_build_wheel"):
backend.prepare_metadata_for_build_wheel(output_dir, config_settings)
else:
wheel_fname = backend.build_wheel(output_dir, config_settings)
(output_dir / "ALREADY_BUILT_WHEEL").write_text(wheel_fname)
unzip_metadata(output_dir / wheel_fname)

def ensure_wheel_is_built(backend, output_dir, config_settings, metadata_dir):
already_built = metadata_dir.parent / "ALREADY_BUILT_WHEEL"
if already_built.exists():
wheel_fname = already_built.read_text()
copy(metadata_dir.parent / wheel_fname, output_dir)
else:
backend.build_wheel(output_dir, config_settings, metadata_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

The functions you're defining in this example are not hook implementations, but they still pass Path objects to the hooks, which is not compliant with the current spec as I understand it. You could str() them when you call the hooks, but I think it would be clearer to consistently use string paths in examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I'll switch it over to using os.path functions and builtin open().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR for this change: #312


and thus expose a totally uniform interface to the rest of the frontend,
with no extra subprocess calls, no duplicated builds, etc. But
obviously this is the kind of code that you only want to write as part
of a private, within-project interface.
of a private, within-project interface (e.g. the given example assumes that
all filesystem paths are supplied as ``pathlib.Path`` instances, rather than
as plain strings).

(And, of course, making the ``metadata`` command optional is one piece
of lowering the barrier to entry, as discussed above.)
Expand Down