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

Implement PEP 517 Build backend #1143

Merged
merged 18 commits into from Oct 12, 2017
Merged

Implement PEP 517 Build backend #1143

merged 18 commits into from Oct 12, 2017

Conversation

@ghost
Copy link

@ghost ghost commented Sep 8, 2017

This PR combines the dist_info PR and the original PR.

Summary:

  • the setuptools.pep517 module is added, which allows querying for setup_requires, among other things (see the PEP for full details)
  • the functions in the added module are the ONLY public API added here. The SetupRequirementsError is not intended to be used except within setuptools.
@ghost ghost mentioned this pull request Sep 10, 2017
@ghost
Copy link
Author

@ghost ghost commented Sep 16, 2017

@jaraco Do you have an ETA on this? I really would like to get this in so that I can work on the pip side.

@jaraco
Copy link
Member

@jaraco jaraco commented Sep 16, 2017

There's a lot going on here. It's going to take me some time to wrap my head around it.

@jaraco
Copy link
Member

@jaraco jaraco commented Sep 16, 2017

The module should probably be named something other than pep517. That's the identifier of the specification... but what is the purpose. Maybe meta.py or build_meta.py?

That module should probably have a docstring. It probably deserves a section in the documentation as well. Adding some documentation would help me understand what its purpose is and how and why someone would use it.

@ghost
Copy link
Author

@ghost ghost commented Sep 16, 2017

It's explained pretty well in the pep, but it can be a little confusing to wrap your head around at first. What pep 517 means is that pip will no longer call setup.py. instead, it will spawn a subprocess that Imports this module and runs the functions. Which particular function it will run and what exactly the function does is outlined pretty well in the pep. There's also an example given in the tests that shows what the pip code will look like.

The main difference between the current process and what this proposes is that rather than running user code first, pep 517 runs then build system code and then the build system code may choose to run user code. So previously setup.py was called, but now setuptools will be called and then setuptools needs to run the setup.py file. There's a bit of trickery that I put in to eliminate multiple processes.

@ghost
Copy link
Author

@ghost ghost commented Sep 16, 2017

I'll update the documentation but let me know if you have any further questions.

@ghost
Copy link
Author

@ghost ghost commented Sep 16, 2017

Also I don't think renaming the module is appropriate because it doesn't just involve metadata.

@ghost
Copy link
Author

@ghost ghost commented Sep 16, 2017

@jaraco Where do you think would be the ideal place to put the documentation? At the bottom of setuptools.txt?

@ghost
Copy link
Author

@ghost ghost commented Sep 25, 2017

@jaraco I added the module docstring as requested. I'm still not sure where the best place is to update the documentation.

@ghost
Copy link
Author

@ghost ghost commented Sep 29, 2017

@jaraco This PEP has been now been accepted (again). Please let me know whether you need any additional explanation.

@ghost
Copy link
Author

@ghost ghost commented Oct 12, 2017

@jaraco Bump.

@jaraco jaraco merged commit 14ea4d1 into pypa:master Oct 12, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jaraco added a commit that referenced this pull request Oct 12, 2017
@jaraco
Copy link
Member

@jaraco jaraco commented Oct 12, 2017

I tried pushing my changes to the PR branch, but it seemed I didn't have permission. For posterity, I think it would be nice if you would pull 8c385a1 into the pep517 branch on your fork so that the commits appear in this PR and the diff more accurately represents the change.

@ghost
Copy link
Author

@ghost ghost commented Oct 12, 2017

I'm sorry about that for some reason the Box wasn't checked. I'll pull the changes as requested.

@ghost
Copy link
Author

@ghost ghost commented Oct 12, 2017

Well, the commit was added but the patch was not updated.

@jaraco
Copy link
Member

@jaraco jaraco commented Oct 16, 2017

Aha. In that case, we can use the merge commit (749c3fe) to see the diff.

@ghost ghost mentioned this pull request Oct 16, 2017
@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

@xoviat any update on this?

@ghost
Copy link
Author

@ghost ghost commented Nov 27, 2017

Setuptools now has full support. I attempted to add support in pip but that became bogged down in disagreement. I've personally become convinced that working on pip is probably not a good use of my time.

@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

so what is the difference on the support between the two sides? (do you have link to setuptools support documentation?) thanks!

@ghost
Copy link
Author

@ghost ghost commented Nov 27, 2017

What are you trying to do?

@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

at the moment just trying to understand what does supporting PEP-518 means for setuptools? e.g. installing build dependencies before invoking build, or also support to store configuration in pyproject.toml, etc [eventually would want to build a wheel with tox and pyproject.toml] (I'm one of the maintainers of the tox project, so trying to understand how things fit together)

@ghost
Copy link
Author

@ghost ghost commented Nov 27, 2017

This is PEP 517. And the only new feature is that you can query setup_requires. The other changes are that you can (should) use the Python functions rather than calling setup.py. See the tests for an example.

@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

never mind 👍

python -c 'import setuptools.build_meta; print(setuptools.build_meta.get_requires_for_build_wheel())'
['setuptools', 'wheel', 'setuptools_scm >= 1.15.6, <2']
@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

@jaraco @xoviat a feedback related to this, https://github.com/pypa/setuptools/blob/master/setuptools/build_meta.py#L83 is always added, so if I version specify any of those, I'll get:

python -c 'import setuptools.build_meta; print(setuptools.build_meta.get_requires_for_build_wheel())'  
['setuptools', 'wheel', 'setuptools_scm >= 1.15.6, <2', 'setuptools >= 38.0.0', 'wheels >= 38.0.0']

note the duplicates, maybe we should add them only if not there afterwards 👍

@ghost
Copy link
Author

@ghost ghost commented Nov 27, 2017

That's not the role of the backend. The frontend is supposed to resolve dependencies, and be that's a valid specification, aside from the typo.

@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

True, but doesn't it seem arbitrary the addition of setuptools and wheel? Independently if it has been specified manually or not?

@ghost
Copy link
Author

@ghost ghost commented Nov 27, 2017

It's not arbitrary. They're both required to build a wheel.

@gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Nov 27, 2017

Fair enough, but wouldn't they be default values only in case the user does not specify any dependencies, in the spirit of PEP-518 in case of no specification.

@ghost
Copy link
Author

@ghost ghost commented Nov 27, 2017

For a setuptools project those will be the required build dependencies. If you add to setup_requires, you would still expect to get setuptools and wheel. That's just the existing de facto standard.

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

Successfully merging this pull request may close these issues.

None yet

3 participants