An ExternalTool subsystem base for downloading external tools. #9625

benjyw · 2020-04-24T01:04:06Z

A streamlined replacement for the existing BinaryToolBase.

The existing code has served us really well for a long time, but has over the years evolved to be convoluted, leans on legacy util code, is biased towards pantsbuild-hosted binaries, has too many moving parts, and is no longer ergonomic.

This new code offers a simple and unified interface for version selection, URL computation,
and digest verification (possibly across multiple known versions). It integrates naturally
with the v2 engine, and supports automatic extraction from archives. Its design and functionality is inspired by the existing code, but cuts down the cruft.

The new ExternalTool subsystem base class is backwards compatible with
BinaryToolBase: A subclass of the latter can be rewritten to subclass the former,
and its options will remain compatible (although there may be deprecation warnings).

This change demonstrates its use via the ClocBinary and Protoc subsystems.
A future change can migrate our other uses of BinaryToolBase.

Note: requires #9624

[ci skip-rust-tests]
[ci skip-jvm-tests]

Useful, e.g., to unpack downloaded external binaries. [ci skip-rust-tests] [ci skip-jvm-tests]

A streamlined replacement for the existing BinaryTool. [ci skip-rust-tests] [ci skip-jvm-tests]

benjyw · 2020-04-24T01:05:19Z

Note to reviewers: You can focus on just the second commit. The first is just #9624.

[ci skip-rust-tests] [ci skip-jvm-tests]

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

Eric-Arellano

Very cool. Thank you!

src/python/pants/backend/codegen/protobuf/subsystems/protoc.py

src/python/pants/backend/codegen/protobuf/python/gen_rules.py

src/python/pants/core/util_rules/external_tool.py

src/python/pants/core/util_rules/external_tool_test.py

src/python/pants/backend/codegen/protobuf/python/gen_rules.py

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

[ci skip-rust-tests] [ci skip-jvm-tests]

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

[ci skip-rust-tests] [ci skip-jvm-tests]

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

stuhood

Thanks!

stuhood · 2020-04-26T18:30:40Z

src/python/pants/core/util_rules/external_tool.py

+    def get_request(self, plat: Platform) -> ExternalToolRequest:
+        """Generate a request for this tool."""
+        return self.generate_request(
+            plat, self.get_options().version, self.get_options().known_versions,
+        )
+
+
+@rule
+async def download_external_tool(request: ExternalToolRequest) -> DownloadedExternalTool:


So, it's currently the case that the Platform is available as a singleton, here:

pants/src/python/pants/engine/platform.py

Lines 32 to 36 in aa2ed8b

# TODO We will want to allow users to specify the execution platform for rules,

# which means replacing this singleton rule with a RootRule populated by an option.

@rule

def materialize_platform() -> Platform:

return Platform.current

... that means that you don't need to take the Platform as an argument as part of the Request, and can instead request the platform singleton.

So your rule signature could probably just be:

@rule async def download_external_tool(request: ExternalToolRequest, platform: Platform) -> DownloadedExternalTool:

The plan shortly (via https://docs.google.com/document/d/1hS0n6vp-hBxy4Xo-q9QA_elofzQtOubjfN4nSeIgp90/edit#heading=h.jj08j7tn7x2d , which I have some initial patches for) is for the Platform to switch from being a singleton to being a Param. That won't change anything from the @rule's perspective, but it will actually allow differentiating requests from one another.

Gotcha. Will make this change.

Hmm, so we need to know the Platform when we create the request, because it can affect the URL, so this would require more restructuring than I think we want right now?

Not a blocker. Thanks!

benjyw · 2020-04-26T18:34:53Z

I'll need to add a select() method for proper backwards compat, so I'll ask you to review again once I've done so. Thanks! 🤦

Eric-Arellano

Great! Thank you Benjy!

Eric-Arellano · 2020-04-27T18:42:16Z

src/python/pants/core/util_rules/external_tool.py

+    Subclass this to configure a specific tool.
+
+
+    Idiomatic use:


Thanks for adding this.

[ci skip-rust-tests] [ci skip-jvm-tests]

cosmicexplorer

Awesome work!!! binary_util.py is horrible to change (and that's partially my fault). Thanks so much for diving in to repair this.

cosmicexplorer · 2020-04-28T05:32:37Z

src/python/pants/core/util_rules/external_tool.py

+            member_type=str,
+            default=cls.default_known_versions,
+            advanced=True,
+            help=f"Known versions to verify downloads against. Each element is a "


Would something like:

dedent(f"""\ ... """)

Potentially be easier to read and change?

Sorry I didn't address these comments Danny. I somehow missed them! Will look now and fix in my current change as needed.

Re this, I'm thinking the rendering code should figure this sort of thing out, as needed?

Oh sure! I meant for the purposes of development, the result might look cleaner. I agree that the rendering code should be figuring out how to display the text.

src/python/pants/core/util_rules/external_tool.py

cosmicexplorer · 2020-04-28T05:38:21Z

src/python/pants/core/util_rules/external_tool.py

+
+
+    A lightweight replacement for the code in binary_tool.py and binary_util.py,
+    which can be deprecated in favor of this.


I would like to add this as a @deprecated_module to those files, then -- we could potentially use deprecation_start_version to delay it if we expect we'll need more than one release cycle to completely remove things.

I know binary_util.py was also used to fetch some tools for rust bootstrapping. We'll probably want to add that before we can remove it entirely.

Yeah, I would like to deprecate, but I'm going to transition all current uses first, to see what yelps.

Yes, sounds great!

cosmicexplorer · 2020-04-28T05:39:40Z

src/python/pants/core/util_rules/external_tool.py

+    ) -> ExternalToolRequest:
+        for known_version in known_versions:
+            try:
+                ver, plat_val, sha256, length = (x.strip() for x in known_version.split("|"))


It might be useful to split this parsing out into a separate method in the future instead of having to hunt for the logic here.

cosmicexplorer · 2020-04-28T05:41:07Z

src/python/pants/core/util_rules/external_tool.py

+    def my_rule(my_external_tool: MyExternalTool):
+      downloaded_tool = await Get[DownloadedExternalTool](
+        ExternalToolRequest, my_external_tool.get_request(Platform.current)
+      )


It would be pretty cool to have this in the official engine README.md. I should also get #9446 in.

[ci skip-rust-tests] [ci skip-jvm-tests]

benjyw added 2 commits April 23, 2020 17:36

Rules to extract archives.

e6a2f7f

Useful, e.g., to unpack downloaded external binaries. [ci skip-rust-tests] [ci skip-jvm-tests]

An ExternalTool subsystem base for downloading external tools.

0faaeac

A streamlined replacement for the existing BinaryTool. [ci skip-rust-tests] [ci skip-jvm-tests]

benjyw requested review from Eric-Arellano, cosmicexplorer and stuhood April 24, 2020 01:04

benjyw added 3 commits April 23, 2020 18:21

Address code review.

a74628d

[ci skip-rust-tests] [ci skip-jvm-tests]

Missing BUILD dep.

bb4887f

[ci skip-rust-tests] [ci skip-jvm-tests]

mypy

0de824d

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

Eric-Arellano reviewed Apr 24, 2020

View reviewed changes

stuhood reviewed Apr 24, 2020

View reviewed changes

src/python/pants/backend/codegen/protobuf/python/gen_rules.py Outdated Show resolved Hide resolved

benjyw added 10 commits April 24, 2020 09:52

Lint

12a276c

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

Merge branch 'archive_rules' into external_tool

bfc9544

[ci skip-rust-tests] [ci skip-jvm-tests]

Merge branch 'master' into archive_rules

3525cc0

[ci skip-rust-tests] [ci skip-jvm-tests]

Merge branch 'archive_rules' into external_tool

97bcbad

[ci skip-rust-tests] [ci skip-jvm-tests]

Merge branch 'master' into external_tool

4111158

[ci skip-rust-tests] [ci skip-jvm-tests]

Address code review.

83d0ef3

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

Lint

26f7816

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

Fix BUILD issue.

a649ae2

[ci skip-rust-tests] [ci skip-jvm-tests]

Fix type.

c2d200a

[ci skip-rust-tests] [ci skip-jvm-tests]

Merge branch 'master' into external_tool

11f94d7

[ci skip-rust-tests] [ci skip-jvm-tests]

benjyw requested review from Eric-Arellano and stuhood April 25, 2020 07:14

Lint

6368669

# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

stuhood approved these changes Apr 26, 2020

View reviewed changes

Eric-Arellano approved these changes Apr 27, 2020

View reviewed changes

benjyw force-pushed the external_tool branch 3 times, most recently from ce0fa04 to d67a3f9 Compare April 27, 2020 20:17

benjyw force-pushed the external_tool branch 2 times, most recently from abfea9f to 626ead4 Compare April 27, 2020 22:41

Backwards compat

34c4505

[ci skip-rust-tests] [ci skip-jvm-tests]

benjyw force-pushed the external_tool branch from 626ead4 to 34c4505 Compare April 27, 2020 23:02

Eric-Arellano approved these changes Apr 28, 2020

View reviewed changes

Fix test.

c031249

[ci skip-rust-tests] [ci skip-jvm-tests]

cosmicexplorer approved these changes Apr 28, 2020

View reviewed changes

Fix test

eb5500c

[ci skip-rust-tests] [ci skip-jvm-tests]

benjyw merged commit 9760c73 into pantsbuild:master Apr 28, 2020

benjyw deleted the external_tool branch April 28, 2020 16:20

benjyw mentioned this pull request Apr 28, 2020

Overhaul BinaryUtil to play nicely with v2 #7790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

An ExternalTool subsystem base for downloading external tools. #9625

An ExternalTool subsystem base for downloading external tools. #9625

benjyw commented Apr 24, 2020 •

edited

Loading

benjyw commented Apr 24, 2020

Eric-Arellano left a comment

stuhood left a comment

stuhood Apr 26, 2020

benjyw Apr 26, 2020

benjyw Apr 26, 2020

stuhood Apr 27, 2020

benjyw commented Apr 26, 2020

Eric-Arellano left a comment

Eric-Arellano Apr 27, 2020

cosmicexplorer left a comment

cosmicexplorer Apr 28, 2020

benjyw Apr 29, 2020

cosmicexplorer Apr 29, 2020

cosmicexplorer Apr 28, 2020

benjyw Apr 29, 2020

cosmicexplorer Apr 29, 2020

cosmicexplorer Apr 28, 2020

cosmicexplorer Apr 28, 2020

	# TODO We will want to allow users to specify the execution platform for rules,
	# which means replacing this singleton rule with a RootRule populated by an option.
	@rule
	def materialize_platform() -> Platform:
	return Platform.current



		A lightweight replacement for the code in binary_tool.py and binary_util.py,
		which can be deprecated in favor of this.

An ExternalTool subsystem base for downloading external tools. #9625

An ExternalTool subsystem base for downloading external tools. #9625

Conversation

benjyw commented Apr 24, 2020 • edited Loading

benjyw commented Apr 24, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Apr 26, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Apr 24, 2020 •

edited

Loading