Skip to content

Support requirement parsing in setuptools.build_meta#1720

Merged
pganssle merged 5 commits into
pypa:masterfrom
pganssle:fix_setup_meta
Mar 16, 2019
Merged

Support requirement parsing in setuptools.build_meta#1720
pganssle merged 5 commits into
pypa:masterfrom
pganssle:fix_setup_meta

Conversation

@pganssle

@pganssle pganssle commented Mar 16, 2019

Copy link
Copy Markdown
Member

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

Per GH pypa#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.
This fixes GH pypa#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 pypa#1682
@pganssle pganssle requested a review from jaraco March 16, 2019 17:05
Comment thread setuptools/build_meta.py
@pganssle

pganssle commented Mar 16, 2019

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

pganssle commented Mar 16, 2019

Copy link
Copy Markdown
Member Author

@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 March 16, 2019 17:44
@benoit-pierre

Copy link
Copy Markdown
Member

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 benoit-pierre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, apart from the dynamic import.

Since pkg_resources is imported elsewhere anyway, we don't get much
value out of porting the requirement parser locally.
@pganssle

Copy link
Copy Markdown
Member Author

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

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

Copy link
Copy Markdown
Member

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

@pganssle

pganssle commented Mar 16, 2019

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Member

Removing all of setup_requires.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants