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

Default to using the entire tool lockfile. (Cherry-pick of #18793) #18807

Merged
merged 1 commit into from Apr 24, 2023

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Apr 23, 2023

The initial implementation of installing tools from regular user
lockfiles was designed with the feature of subsetting from a
larger lockfile in mind.

Therefore, we introduced the requirements option, which
enumerated the requirements to be installed for the tool,
and defaulted to the requirements in the built-in lockfile
provided with Pants.

However now that users have started experimenting with this
feature, it turns out to be quite laborious - in the more common
case where you create a custom lockfile that is intended to be
used in its entirety, you still have to update requirements
(e.g., to tweak versions or add extra requirements) even though
you already enumerated the requirements as inputs to lockfile
generation.

This PR changes the semantics of the requirements option.
It now defaults to an empty list, indicating that the entire lockfile
should be used. Only if you truly need to subset do you enumerate
requirements (which can be versionless, since the lockfile provides
the version).

This has the side-effect of no longer validating that a custom tool
lockfile provides a tool version that Pants believes it can handle.
But this nannying has also been found by users to be annoying, and
is an example of Pants getting in the way (users were just
bypassing this check by updating versions in requirements, which
seems like unnecessary hoop-jumping).

Now if a user-provided tool version doesn't work, e.g., because a
CLI arg is no longer available, or other semantics have changed,
then the user will get an error, or unwanted behavior, from the tool,
and it will be up to them to YOLO that.

This change doesn't require deprecation, since the requirements
option has not been released yet.

The initial implementation of installing tools from regular user
lockfiles was designed with the feature of subsetting from a
larger lockfile in mind.

Therefore, we introduced the `requirements` option, which
enumerated the requirements to be installed for the tool,
and defaulted to the requirements in the built-in lockfile
provided with Pants.

However now that users have started experimenting with this
feature, it turns out to be quite laborious - in the more common
case where you create a custom lockfile that is intended to be
used in its entirety, you still have to update `requirements`
(e.g., to tweak versions or add extra requirements) even though
you already enumerated the requirements as inputs to lockfile
generation.

This PR changes the semantics of the `requirements` option. 
It now defaults to an empty list, indicating that the entire lockfile
should be used.  Only if you truly need to subset do you enumerate
`requirements` (which can be versionless, since the lockfile provides
the version).

This has the side-effect of no longer validating that a custom tool
lockfile provides a tool version that Pants believes it can handle. 
But this nannying has also been found by users to be annoying, and
is an example of Pants getting in the way (users were just
bypassing this check by updating versions in `requirements`, which
seems like unnecessary hoop-jumping).

Now if a user-provided tool version doesn't work, e.g., because a
CLI arg is no longer available, or other semantics have changed,
then the user will get an error, or unwanted behavior, from the tool,
and it will be up to them to YOLO that.

This change doesn't require deprecation, since the `requirements`
option has not been released yet.
@huonw
Copy link
Contributor Author

huonw commented Apr 23, 2023

Similar to #18806, I noticed #18625 affects 2.16.x, but #18793 wasn't tagged for cherrypicking.

@jsirois
Copy link
Member

jsirois commented Apr 23, 2023

I'll let @benjyw and @kaos review this - they're closer to the code and release this week and I honestly don't know if this meets the bar.

@kaos
Copy link
Member

kaos commented Apr 24, 2023

I'd prefer to hold picking this until we can confirm wether it actually resolves #18625 in which case we could treat it as bugfix. As is it's labeled as a user api change, and I'd rather we maintain a somewhat strict policy of only picking bug fixes.

@huonw
Copy link
Contributor Author

huonw commented Apr 24, 2023

For this specific change, I suspect the assumption in:

This change doesn't require deprecation, since the requirements
option has not been released yet.

means it is nicer if it is picked: without the cherrypick, the underlying feature will be released (in 2.16) and #18793 will go out in 2.17, and thus require adjustment/deprecation.

Per #18806 (comment), sounds like it probably would've been better for me to ask about these on Slack instead of just jumping in. Hopefully we can have a productive discussion here anyway 😄

@kaos
Copy link
Member

kaos commented Apr 24, 2023

Seems I'm the only one awake at the moment. I'm not entirely up to speed on the changes them selves so have merely replied with what we want to pick (bug fixes, in general). These changes may very well be something we want/should be picking. I'll let @benjyw et al weigh in on that.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Apr 24, 2023

Yep, as before this should have been cherry-picked by me, or at least tagged. I'm distracted by PyCon... Thanks @huonw !

@huonw huonw merged commit 3ab1dde into pantsbuild:2.16.x Apr 24, 2023
17 checks passed
@huonw huonw deleted the cherry-pick-18793-to-2.16.x branch April 24, 2023 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants