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

Support requirement parsing in setuptools.build_meta #1720

Merged
merged 5 commits into from Mar 16, 2019

Conversation

Projects
None yet
2 participants
@pganssle
Copy link
Member

commented Mar 16, 2019

Summary of changes

In the original setup.py interface, setup_requires would also accept some newline-delimited string format (including comments), but setuptools.build_meta assumes that you have passed a list when converting setup_require to get_requires_for_*.

In this PR, I've added this behavior to setuptools.build_meta, but an argument could be made for restricting this to setuptools.build_meta:__legacy__ and requiring a setup_requires to take a list in the main build_meta.

Closes #1682.

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

pganssle added some commits Mar 16, 2019

Add failing test for setup_requires
Per GH #1682, with setuptools.build_meta we are not properly handling
the situation where setup_requires is actually a newline-delimited
string rather than a list, which is supported by setup.py interface.

This adds several failing (and some passing) tests for how
setup_requires is handled by setuptools.build_meta.
Add requirement parsing in setuptools.build_meta
This fixes GH #1682 by porting the pkg_resources requirement parsing
logic into setuptools.build_meta, so that all valid requirement
specifiers passed to setup_requires will be added to the
get_requires_for_build_* function outputs.

Fixes GH #1682

@pganssle pganssle requested a review from jaraco Mar 16, 2019

@pganssle

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

Another question here (which @benoit-pierre commented on while I was typing this up) is whether we should actually just use pkg_resources rather than port this requirement parser like this. I had three reasons for doing this:

  1. I got the impression that we may want to move away from the tight integration with pkg_resources in the future (to the extent possible), and in the hip and modern build_meta code, it seemed prudent to avoid it.
  2. The pkg_resources function yields a Requirement object rather than a list of strings.
  3. My understanding is that in many situations even importing pkg_resources is expensive, so if we can avoid it we should (though I'm pretty sure it will be imported in setup.py anyway).

The downside of this particular port is that it might get out of phase with the pkg_resources version, and if we ever want to move the requirement parsing out of pkg_resources into a central place, it will be harder to find this duplicate in a search-replace.

I'm OK with switching to the pkg_resources version if that's the consensus.

@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

IMHO it's better to keep it simple, reuse the existing code, rather than duplicate it in favor of an hypothetical split of pkg_resources/setuptools (which I really don't see happening anytime soon). And pkg_resources will be imported anyway.

@pganssle

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

@benoit-pierre OK, I'm convinced, I've updated the PR.

I went with the mildly-import-conservative route of importing pkg_resources in the only function that actually uses it (though I think it probably gets imported when resolving Distribution anyway).

@pganssle pganssle requested a review from benoit-pierre Mar 16, 2019

@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I went with the mildly-import-conservative route of importing pkg_resources in the only function that actually uses it (though I think it probably gets imported when resolving Distribution anyway).

Let's not cargo cult it: pkg_resources is always imported as a result of importing setuptools, so there's no point in this dynamic import.

@benoit-pierre
Copy link
Member

left a comment

LGTM, apart from the dynamic import.

Use pkg_resources.parse_requirements in build_meta
Since pkg_resources is imported elsewhere anyway, we don't get much
value out of porting the requirement parser locally.

@pganssle pganssle force-pushed the pganssle:fix_setup_meta branch from 48e796e to 5efdf81 Mar 16, 2019

@pganssle pganssle requested a review from benoit-pierre Mar 16, 2019

@pganssle

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

@benoit-pierre Fair point, I've made the requested change.

@pganssle pganssle merged commit 4cd3da0 into pypa:master Mar 16, 2019

5 checks passed

codecov/patch 100% of diff hit (target 82.67%)
Details
codecov/project 82.69% (+0.01%) compared to c27c705
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I also think that calling setup.py to fetch setup_requires should be limited to the __legacy__ only backend.

@pganssle

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

@benoit-pierre You mean just the requirement parsing part of it, or all of the setup_requires thing? Removing it from build_meta would be a backwards-incompatible change.

If we're going down that route, just deprecating setup_requires entirely is probably a better course of action.

@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Removing all of setup_requires.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.