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

GenerateToolLockfileSentinel encouraged to use language-specific subclasses #16449

Merged

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Aug 9, 2022

Closes #16443, which is necessary for the per-resolve config project from https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit# to work properly. See #16420 (comment).

As discussed on the ticket, this is not a very elegant solution. But we don't have the rule graph introspection mechanism necessary to automagically figure out what type each tool is, and that also seems brittle.

As explained in this PR's updated docs, plugin authors don't have to technically make this change.

[ci skip-rust]
[ci skip-build-wheels]

…bclasses

[ci skip-rust]

[ci skip-build-wheels]
This required moving the rule to resolve ResolvePexConfig to avoid a circular import
@Eric-Arellano
Copy link
Contributor Author

Two commits are distinct

all_python_tool_resolve_names = tuple(
sentinel.resolve_name
for sentinel in union_membership.get(GenerateToolLockfileSentinel)
if issubclass(sentinel, GeneratePythonToolLockfileSentinel)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only part of the rule that is new

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mod red CI / rule graph issue.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano enabled auto-merge (squash) August 9, 2022 16:32
@Eric-Arellano Eric-Arellano merged commit 28d756e into pantsbuild:main Aug 9, 2022
@Eric-Arellano Eric-Arellano deleted the python-vs-jvm-tool-locks branch August 9, 2022 17:15
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
…bclasses (pantsbuild#16449)

Closes pantsbuild#16443, which is necessary for the per-resolve config project from https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit# to work properly. See pantsbuild#16420 (comment). 

As discussed on the ticket, this is not a very elegant solution. But we don't have the rule graph introspection mechanism necessary to automagically figure out what type each tool is, and that also seems brittle.

As explained in this PR's updated docs, plugin authors don't have to technically make this change.

[ci skip-rust]
[ci skip-build-wheels]
@huonw huonw mentioned this pull request Jul 12, 2023
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.

Distinguish between Python vs JVM tool lockfile requests
2 participants