make --python-version override python_version in requirement markers

[chadrik]

This PR resolves half of issue #6117.

This PR ensures that if a user provides the --python-version cli option (e.g. to pip download), that python_version markers are evaluated as expected. The other half of the issue -- doing the same for --platform -- seems a bit more difficult (e.g. how can I reliably break down a value passed to --platform into sys.platform, platform.system(), etc?). This PR sets up the resolution of the platform issue by exposing an environment dictionary that can be used to override any part of the marker env.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[chadrik]

Hi, any thoughts on this? I'm using this change and it's working great.

[chadrik]

Hi, just checking in. It's been a month without any response.

[cjerdonek]

I took a look at #6117 the other day and think I like the overall approach of this PR. I can try to find time to review within the next week or so.

[cjerdonek]

By the way, since you put thought into a similar issue, do you have any thoughts on the question I asked here? #6371 (comment)

[chadrik]

By the way, since you put thought into a similar issue, do you have any thoughts on the question I asked here?

Sorry, I don't have much useful input, other than that I find the the way that versions are currently passed (e.g. 27, 34) confusing, and in my code I have to add periods anyway to make it compatible with packaging.markers.Marker, but I think there are other parts of the code that do rely on the non-period-delimited versions.

[cjerdonek]

I gave this issue / PR some thought, and here's how I think you should proceed on this. I've been doing some work that relates to this, so I want to be sure this will mesh well with other changes.

So what I'm thinking is--

• Next to where --python-version is defined inside cmdoptions.py, add a helper function called something like python_version_to_version_info() that converts the option value to a Tuple[int, ...]. You can add unit tests for this inside tests/unit/test_options.py.

• Instead of adding an environment variable to the functions needed for this PR, add a py_version_info argument like CandidateEvaluator has here:
  

  pip/src/pip/_internal/index.py

  Lines 302 to 316 in eeb74ae

  	class CandidateEvaluator(object):
  	"""
  	Responsible for filtering and sorting candidates for installation based
  	on what tags are valid.
  	"""
  	def __init__(
  	self,
  	valid_tags, # type: List[Pep425Tag]
  	prefer_binary=False, # type: bool
  	allow_all_prereleases=False, # type: bool
  	py_version_info=None, # type: Optional[Tuple[int, ...]]
  	ignore_requires_python=None, # type: Optional[bool]
  	):

  
  You would be passing the return value of python_version_to_version_info(options.python_version).

• On the InstallRequirement class, add a make_new_environment(extra) method that uses self.py_version_info (instead of your in-functionenv()).

The main reason for not adding environment right now is that while I think something like that would be useful, I think it needs to be thought through some more so we don't e.g. wind up having too many different arguments containing overlapping information. I think some consolidation of arguments will need to happen, but we'll probably want some kind of object for that that uses the "raw" information rather than computed to a dict.

[cjerdonek]

FYI, I posted PR #6539 which will help with this.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[cjerdonek]

@chadrik Would you be willing to update your PR for this? There have been some changes in master, which should help you update your PR along the lines I suggested in this comment above: #6357 (comment)

[chadrik]

Sorry, I don’t think I’ll have any time for a few more weeks.

…

On Sat, Jun 8, 2019 at 7:50 PM Chris Jerdonek ***@***.***> wrote: @chadrik <https://github.com/chadrik> Would you be willing to update your PR for this? There have been some changes in master, which should help you update your PR along the lines I suggested in this comment above: #6357 (comment) <#6357 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6357?email_source=notifications&email_token=AAAPOE4BXSRVC6YQFINYIVLPZRVYFA5CNFSM4HASFAJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXICL2A#issuecomment-500180456>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPOE2CTAOJV5S2QSB7FSDPZRVYFANCNFSM4HASFAJA> .

[cjerdonek]

Ping, @chadrik. :)

[chrahunt]

Hello! Thanks so much for your contribution. It has been some time since this has seen activity, so I will close this PR. This is not saying these changes are not useful, just that it is not something we're considering actively worked on right now.

@chadrik or anyone else interested in picking this up, please don't hesitate to open a new PR. The approach outlined by @cjerdonek should still be mostly compatible with what is in the master branch.