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

Use global instance of PythonSetup for resolve requirements task base #7672

Merged
merged 2 commits into from May 9, 2019

Conversation

Projects
None yet
3 participants
@CMLivingston
Copy link
Contributor

commented May 7, 2019

Fix for #7625. The resolve requirements task base module was using a scoped instance of PythonSetup, breaking the ./pants repl --python-setup-interpreter-constraints=<constraints> <target> usage pattern.

Solution

Use the global instance of PythonSetup so we can still use global options that cut across all interpreter selection-related mechanisms.

Result

Users that set non-scoped options on PythonSetup are once again able to apply them globally across tasks that use PythonSetup.

Christopher Livingston added some commits May 7, 2019

Christopher Livingston
Use global PythonSetup instance in resolve_requirements task base to …
…ensure cross-cutting global options make it to their relevant tasks

@CMLivingston CMLivingston requested review from jsirois and benjyw May 7, 2019

@jsirois

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Don't forget to work off your fork if pantsbuild/pants - personal branches should not be pushed to pantsbuild/pants.

@jsirois

jsirois approved these changes May 8, 2019

@CMLivingston

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

ah shoot, sorry about that. My computer got wiped and I misconfigured my remotes. I'll delete it after this lands.

@CMLivingston

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I am unable to re-kick the two failed travis builds. They appear unrelated, would someone mind restarting those for me please?

@stuhood

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@CMLivingston : They're looking green now. Can ignore the push build.

@stuhood

stuhood approved these changes May 8, 2019

PythonNativeCode.scoped(cls),
PythonSetup.scoped(cls),

This comment has been minimized.

Copy link
@stuhood

stuhood May 8, 2019

Member

I think that this represents a breaking change... maybe one that we're ok with though.

A middle ground might be to leave behind the old scoped dep as deprecated (via PythonSetup.scoped(cls, removal_version=.., removal_hint=..)), and just not use it?:

@classmethod
def scoped(cls, optionable, removal_version=None, removal_hint=None):
"""Returns a dependency on this subsystem, scoped to `optionable`.
:param removal_version: An optional deprecation version for this scoped Subsystem dependency.
:param removal_hint: An optional hint to accompany a deprecation removal_version.
Return value is suitable for use in SubsystemClientMixin.subsystem_dependencies().
"""
return SubsystemDependency(cls, optionable.options_scope, removal_version, removal_hint)

@CMLivingston

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

After discussing with @stuhood, I'm going to leave as is due to the option overriding going on if we try to deprecate the scoped dependency alongside a global subsystem dependency.

@CMLivingston CMLivingston merged commit 87e7d2a into master May 9, 2019

1 of 2 checks passed

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

@CMLivingston CMLivingston deleted the clivingston/7625 branch May 9, 2019

@stuhood stuhood added this to the 1.16.x milestone May 9, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Have marked this for 1.16.x.

stuhood added a commit that referenced this pull request May 19, 2019

Use global instance of PythonSetup for resolve requirements task base (
…#7672)

* Use global PythonSetup instance in resolve_requirements task base to ensure cross-cutting global options make it to their relevant tasks

* Update tests to test with proper option scoping for global python-setup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.