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

Add pants_runtime_python_version global option #7363

Merged

Add expected input format for option

  • Loading branch information...
Eric-Arellano committed Mar 12, 2019
commit 307ac94f45f2fa83df81188abdbd51257bb78455
@@ -126,7 +126,7 @@ def register_bootstrap_options(cls, register):
# version. It too may be grepped out of pants.ini to be used by things like setup scripts,
# runner scripts, IDE plugins, etc. in order to select which interpreter to use.
register('--pants-engine-python-version', advanced=True,
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

IMO, just pants_python_version would be sufficient here, because global options are namespaced, and are not recursive into other subsystem namespaces.

But I would move more of what is in the comment into the help for this option (and if you're feeling ambitious, maybe do the same help expansion for --pants-version)?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

(I think the whole comment should be moved into the help)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 12, 2019

Author Contributor

Moved the comment entirely into help, along with for --pants-version! Thanks for the suggestion.

Regarding pants_python_version, I'm concerned that it wouldn't be clear enough this is what Pants uses for itself and is not what Pants uses for running the user's own code. I want to make sure it's crystal clear this is different than --python-setup-interpreter-constraints. Also not super concerned about the option taking extra characters to type, as this is supposed to be pinned in pants.ini. But definitely fine with renaming if others prefer pants_python_version! Thoughts from others?

help='Use this Python version to run Pants.')
help='Use this Python version to run Pants. Valid values: 2.7, 3.6, or 3.7.')
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

With this implementation (reading it as a bootstrap option), I actually would suggest considering:

class PantsEnginePythonVersion(enum(['2.7', '3.6', '3.7'])):
  @memoized_classproperty
  def current(cls):
    current_python_version = '.'.join(map(str, sys.version_info[0:2]))
    return cls(current_python_version)
# ...
  register('--pants-engine-python-version', advanced=True, type=PantsEnginePythonVersion,
           default=PantsEnginePythonVersion.current,
           help='Use this Python version to run Pants.')

(for the record, we do this in the native backend):

class Platform(enum(all_normalized_os_names())):
# TODO: try to turn all of these accesses into v2 dependency injections!
@memoized_classproperty
def current(cls):
return cls(get_normalized_os_name())

and we can then do:

    pants_engine_python_version = global_bootstrap_options.pants_engine_python_version
    if pants_engine_python_version != PantsEnginePythonVersion.current:
      # raise

Or at least use choices here to avoid the docs falling out of sync with the code. But I like the enum because then in addition to the above, we can do PantsEnginePythonVersion.iterate_enum_variants() to get a list of all the supported versions anywhere else in the codebase that we might want to do that, for example.

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

This shouldn't be an enum, because we don't know what values users will use for it. Anything that could legally be passed to virtualenv, essentially.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 12, 2019

Author Contributor

I've gone back and forth on if we should enumerate all the supported versions here or not..

--

This question speaks to a related check that we're going to need to add: in pants_loader.py checking for a valid interpreter. This was suggested first by Kris here #6062 (comment). It will assert that they're using a supported Py version, i.e. not Py2.6 or 3.0-3.5 (soon 3.6+).

The reason this belongs in pants_loader.py and not here in global_options.py or options_initializer.py is that we cannot make any assumptions about them using this option and how they're setting the interpreter to run Pants with. Even if they completely circumvent this option / our setup repo and try running Pants with something like Py3.4, we need to run the assertion.

--

I think we probably want to avoid, then, enumerating the options here, as that's the job of pants_loader.py.

Instead, my original intent was to give an example of what this value is supposed to look like, i.e. that it should be the major and minor number like 2.7 or 3.6. We need it to be like this because of the check here: https://github.com/pantsbuild/pants/pull/7363/files#diff-c30fdc263a0a6d8522b90488e2a264d5R112.


register('--plugins', advanced=True, type=list, help='Load these plugins.')
register('--plugin-cache-dir', advanced=True,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.