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

Only depend on subprocess32 if running in python2. #5847

Merged
merged 1 commit into from May 21, 2018

Conversation

Projects
None yet
2 participants
@benjyw
Copy link
Contributor

benjyw commented May 20, 2018

subprocess32 will not install in python3.

Granted, Pants currently only runs in python2. However it is possible for python3 code in some repo to depend on Pants as a library (whether or not the Pants methods it calls will work in python3 is of course questionable). Nonetheless, in this case we don't want to attempt to install subprocess32 as a transitive dep.

And besides, it's good hygiene for the day we make Pants py3 compatible.

@benjyw benjyw requested review from jsirois and kwlzn May 20, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented May 20, 2018

I'm not sure this works in the future world. Namely this https://github.com/pantsbuild/pants/blob/master/src/python/pants/util/BUILD#L102 will attempt to generate a PythonRequirement which will, in turn, attempt a Requirement.parse which will... I'm being lazy from my phone and I don't know. Return None maybe or set a field indicating the constraint? Basically I'm prompting to know if your sure the pants/pex modeling of this requirements.txt edit works out.

@jsirois
Copy link
Member

jsirois left a comment

Long term doubt aside, this change is locally correct, does not break CI and expresses the right thing in the distribution we export, so LGTM but maybe worth a comment here or in the BUILD dep consumer I pointed out.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented May 21, 2018

I've verified that Requirement.parse("subprocess32==3.2.7 ; python_version>'3'") is fine.

It parses it and places the python version constraint into the marker field of the returned pkg_resources.Requirement instance.

@benjyw benjyw merged commit 39be10a into pantsbuild:master May 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:qualify_subprocess32_dep branch May 21, 2018

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