[WIP] Add --interpreter-compatibility option to repl goal #6234
Conversation
| @@ -30,6 +31,8 @@ def register_options(cls, register): | |||
| help='The IPython REPL entry point.') | |||
| register('--ipython-requirements', advanced=True, type=list, default=['ipython==1.0.0'], | |||
| help='The IPython interpreter version to use.') | |||
| register('--interpreter-compatibility', advanced=True, type=list, default=[], | |||
There was a problem hiding this comment.
After exploring options further, I noticed that these are namespaced to --repl-py, so we might be able to shorten this to just --compatibility. Keeping these options short and sweet makes them easier to use for the end developer. The conceptual model should carry over from the target-level compatibility settings, but I would like some input from other Pants contributors before we change it.
| @@ -46,8 +49,19 @@ def setup_repl_session(self, targets): | |||
| entry_point = self.get_options().ipython_entry_point | |||
| else: | |||
| entry_point = 'code:interact' | |||
| # Merge interpreter constraints | |||
There was a problem hiding this comment.
Micro-nit: period after comments please.
| repl_constraints = self.get_options().interpreter_compatibility | ||
| target_constraints = [constraint for target in targets for constraint in target.compatibility] | ||
| if repl_constraints and target_constraints: | ||
| interpreter_constraints = [",".join(pair) for pair in zip(repl_constraints, target_constraints)] |
There was a problem hiding this comment.
Looks like merging will become a bit more intricate than first expected. We want a single string like you have here so that we AND the constraints, however we need to do things like de-duplicate interpreter class (CPython) and this will currently fail to parse: CPython>=2.7,<3,CPython>3. Also, this is a sizable chunk that would be a great opportunity for abstraction into a helper instance method, say merge_interpreter_constraints(targets) or similar.
There was a problem hiding this comment.
I will follow up with you about this offline.
|
Why is a new option needed? IOW, why is python_setup = PythonSetup.global_instance()
custom_repl_constraints_supplied = python_setup.get_options().is_flagged('interpreter_constraints')
... |
|
AFAICT the new option is needed to either merge or override compatibility constraints at the target level. The use case is that we have internal 2/3 compatible libraries that specify |
I commandeered @alanbato's PR at #6250 ## Problem The original issue is documented at #6081, an attempt to solve this was #6234, but we've decided to take another path instead. ## Solution Instead of relying on interpreter constraints when building a pex in the repl task, we're checking if the --python-setup-interpreter-constraints is flagged. If so, it overrides the interpreter filters at interpreter selection time. ## Result Users should now be able to further filter the python interpreter to use even if the task does not declare an option to do so (like in the repl task)
Problem
As explained in #6081, currently there isn't a way to further constrain the python interpreter version selection when firing up a repl session.
Solution
This PR includes the addition of a new option under the repl goal scope, and also the merging of the three ways of specifying interpreter version constraints (
pants.ini, by-target, by options).