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

PEP 517: Add prepare_build_files hook #277

Merged
merged 4 commits into from
Jun 5, 2017

Conversation

takluyver
Copy link
Contributor

`setuptools_scm <https://github.com/pypa/setuptools_scm>`_, this may include
extracting some information from a version control system.
The ``build_wheel`` hook will subsequently be run from the ``build_directory``
populated by this hook.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably obvious, but should be stated explicitly, that build_wheel run from the directory created by this step must produce an identical wheel to the one produced by running build_wheel in the original directory.

Should it also be stated that pyproject.toml must be copied unchanged (to prohibit the sort of silliness I described on distutils-sig)?

But overall, I like this version. It's clean and straightforward.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for stating explicitly that pyproject.toml must be exported unchanged, and that the build output should be the same regardless of whether or not the frontend actually calls this hook.

pep-0517.txt Outdated
Optional. If this hook is not defined, the copying step will be skipped, and the
``build_wheel`` hook invoked in the original source directory. In this case, the
backend is responsible for not cluttering up the source directory.

Mandatory.
Copy link
Member

Choose a reason for hiding this comment

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

I suspect "Mandatory" is misplaced or should be removed?

pep-0517.txt Outdated

Optional. If this hook is not defined, the copying step will be skipped, and the
``build_wheel`` hook invoked in the original source directory. In this case, the
backend is responsible for not cluttering up the source directory.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like this. I can't see a problem with it, but I've a nagging feeling that I'd prefer the front end to be in charge of the "copy or not" decision. Also, it's not necessarily the backend that clutters the source directory, it could be the user. But to repeat, I can't think of an actual issue here, so unless someone else comes up with a specific way that this could break something, I'm not going to object.

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 don't feel strongly about this, my rationale is just that if the backend knows it can build a wheel without creating clutter in the source directory (as flit does), then we can save copying the files. But the normal operation should be installing from a pre-built wheel anyway, so I don't think performance is that critical for this step.

@ncoghlan ncoghlan merged commit 64442c8 into python:master Jun 5, 2017
@ncoghlan
Copy link
Contributor

ncoghlan commented Jun 5, 2017

I went ahead and merged this to bring the published PEP in line with current distutils-sig discussion, but will add a couple of comments inline for things I think are still a bit ambiguous.

def build_sdist(sdist_directory, config_settings):
...

Must build an unpacked source distribution in the specified
Copy link
Contributor

Choose a reason for hiding this comment

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

With the sdist now being unpacked, a better name for the hook is likely to be export_sdist.

It's also not entirely clear if the sdist contents should be exported directly into the supplied sdist_directory, or if the backend should be creating an appropriately named subdirectory.

My inclination would be to leave the directory naming up to the frontend, and have the backend just dump the sdist contents into whichever path is given (as happens with the wheel metadata export)

@takluyver takluyver deleted the 517-prep-build-files branch June 8, 2017 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants