-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 #1039
Conversation
@jaraco This should be okay to go if the tests pass. It's not bulletproof but pip can always fall back. |
Can you provide a link to the pep? Why use |
Wow, it looks like it's been accepted. https://www.python.org/dev/peps/pep-0517/ @jaraco I'll rename the module. I think pip was originally going to have it's own setuptools backend but if we put the backend in the setuptools repository, then pip can just require the latest setuptools by default and then execute the backend according to the standards. In other words, it move all of the monkeypatching and hackery out of pip and into setuptools, where it belongs. |
This change brings setuptools into very basic compliance with PEP 517.
@ncoghlan Can you specify the additonal concerns for out-of-tree builds? |
|
So just as a reference we need to use |
setuptools/dist.py
Outdated
self.fetch_build_eggs(attrs['setup_requires']) | ||
if skip_install_eggs: | ||
raise SetupRequirementsError(attrs['setup_requires']) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the else block here.
setuptools/dist.py
Outdated
@@ -25,6 +25,14 @@ | |||
from .py36compat import Distribution_parse_config_files | |||
|
|||
|
|||
skip_install_eggs = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable appears to be undocumented and untested. It could be optimized away.
I wonder if instead of setting a global flag that tweaks the inner behavior of the Distribution object for all users if it wouldn't be better for the caller that would use this functionality to instead create a:
class Distribution(setuptools.dist.Distribution):
def fetch_build_eggs(self, eggs):
raise SetupRequirementsError(...)
It's not yet clear to me where a user or application would hook into this behavior.
Also, the name 'skip_install_eggs' doesn't match the behavior. It's 'fail on install eggs'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is for internal use only. The public api is get_requires_for_build_wheel
What's the status of this PR? It's still tagged WIP, yet one comment says it's good to go. Other comments still say "we need", suggesting there's more work to be done. I've added some comments about the implementation. I'm generally fine with new pep517 module, but I'm reluctant to add implicitly-defined hooks that complicate the (already complicated) behavior. I've suggested another possible approach which may prove simpler, but first, I think we need to understand how it is that this behavior is intended to be used. |
The reason for this is that the PEP was modified after it was initially accepted. It's not currently acceptable to merge. I'm working on the pip side of the chain first and will then update this. |
@jaraco Let's go ahead and get this in here as the pip people seem to be taking their time. |
@@ -25,6 +25,14 @@ | |||
from .py36compat import Distribution_parse_config_files | |||
|
|||
|
|||
_skip_install_eggs = False | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So again I want to emphasize that this is an internal setuptools implementation detail that should not be used outside of setuptools.
def get_requires_for_build_sdist(config_settings=None): | ||
config_settings = fix_config(config_settings) | ||
return get_build_requires(config_settings) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two get_requires
functions are the public API. They will be used by pip to determine the setup requirements.
I added some tests that should help clarify this matter. Let me know if you have any other questions. |
def prepare_metadata_for_build_wheel(metadata_directory, config_settings=None): | ||
sys.argv = sys.argv[:1] + ['dist_info', '--egg-base', metadata_directory] | ||
_run_setup() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will start working once pypa/wheel#190 is merged but there is no need to wait for that.
This complies with the pep that will probably be adopted in any case, and doesn't break any existing functionality.