Dedup and merge reqs to pass into pip #15
Conversation
uranusjr
force-pushed
the
dedup-pip-install-arguments
branch
from
60cbe6d
to
851d756
Compare
| def _load_pyproject(source_dir): | ||
| with open(os.path.join(source_dir, 'pyproject.toml')) as f: | ||
| pyproject_data = pytoml.load(f) | ||
| buildsys = pyproject_data['build-system'] | ||
| return buildsys['requires'], buildsys['build-backend'] | ||
|
|
||
|
|
||
| REQUIREMENT_PATTERN = re.compile(r'^(?P<name>[^~><=!]+)(?P<spec>.*)$') | ||
|
|
There was a problem hiding this comment.
should we use instead here the pep 440 parser instead of the naive regex? I've used it when I hit this edge case in tox to resolve the conflict. Tbh though I think we should really warn if the two versions don't match exactly. it's likely a bug.
There was a problem hiding this comment.
@pfmoore @takluyver Thoughts?
There was a problem hiding this comment.
I suspect so, but I don't use this class in pip, so I've no real idea.
There was a problem hiding this comment.
AFAICT there are no existing PEP 440 implementations that can tell if a version specifier has conflicts by looking at it. So the only viable thing to do IMO is to just feed it into pip and see what happens.
Theoratically the input here can be an arbitrary PEP 508-compliant string, complete with markers and such, so the complete solution should likely use packaging.requirements to do the parsing. I can do that if @takluyver thinks it’s a good idea to add packaging as a dependency.
There was a problem hiding this comment.
https://packaging.pypa.io/en/latest/requirements/ should allow you to parse, and then you can make the decision if the two versions (if both exists) matches or not. but yeah overall I agree with your assessment. your regex here is a poor mans packaging and if needed we might as well depend on upstream to avoid bugs.
|
I think that what you hit is a bug in the setuptools build backend: I don't think it should be returning the values from
To me, that means that the backend shouldn't include the There may still be some need for deduplication, either in the frontend or the backend. But I'd recommend we focus on fixing the specific problem with setuptools first. |
|
Hmm, going from there indeed setuptools should avoid specifying both wheel and setuptools if they are in the build requires. Can you raise a setuptools issue for this then and link it to this? |
|
Ah, if that's what this is about, I agree. I'd noticed this problem with the setuptools backend, but hadn't got around to (needing to) address it in pip yet. |
|
The downstream issue has been solved with Setuptools 40.6.3. I suggest then this PR can be closed. |
I was trying this on some setuptools-based packages, and find this edge case. If a package specifies both
setup-requiresin setup.py andbuild-system.requiresin pyproject.toml (for compatibility reasons), setuptools’sget_requires_for_build_*hooks would return both of them.This would result in something like
['setuptools>=32.2.2', 'setuptools>=36.6', 'setuptools', 'wheel'], and pip does not like it, failing the build.This PR adds a very naive parser to merge the requirements before passing them to pip. The above example would be converted into
['setuptools>=36.6,32.2.2', 'wheel']. Not the most elegant result, but pip accepts it and does the right thing.I am not sure how this fits in the tests. Feel free to point me a direction and I’ll add it.