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

Make ExternalTool exportable #21258

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

lilatomic
Copy link
Contributor

@lilatomic lilatomic commented Aug 4, 2024

This MR wires ExternalTools into the export machinery.

It exposes them under a separate cli arg --bin. Although it uses some of the same machinery as --resolve, there are several differences that I think support a separate flag (and some separate internal):

  1. ExternalTools don't have a resolve. They therefore must not show up for generating lockfiles. We have to implement this separation interally, so we should probably surface that.
  2. They would be invoked directly (instead of source dist/export/.../activate)
  3. We can put them all in a folder "dist/export/bin" so people can get them all

Closes #21251

Bikeshedding:

  • Do we like --bin (yes)
  • Is putting all the bins in the same dir what we want (yes, we want a "bin" folder with all the binaries. But we have to put each binary's digest in its own folder to prevent collisions of supporting files)
  • [-] Do we want to make all TemplatedExternalTools exportable? We could extend Subsystem.rules and add the UnionRule there (deferred from this MR)

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for taking it on

Do we like --bin

Fine by me!

Is putting all the bins in the same dir what we want

I wonder if we don't, due to two key downsides of putting them all in the same directory:

  • this means that a tool that's more than just a single binary (e.g. READMEs, supporting data files) end up piled into the bin/ directory adjacent to all the other binaries
  • binaries and/or their supporting files can have names that overlap, e.g. hypothetically someone could be using both a helm subsystem and migrating to their own fork of the plugin helm2 or something, and they both might have a helm binary. Or just overlapping on supporting files.

As such, I wonder if we want to stick the Pants option scope in the directory hierarchy as a definitely unique disambiguation, e.g. dist/export/bin/helm/helm or dist/export/helm/bin/helm.

async def export_external_tool(
req: _ExportExternalToolForResolveRequest, platform: Platform, union_membership: UnionMembership
) -> MaybeExportResult:
exporatbles = ExportableTool.filter_for_subclasses(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exporatbles = ExportableTool.filter_for_subclasses(
exportables = ExportableTool.filter_for_subclasses(

@lilatomic
Copy link
Contributor Author

I wonder if we don't, due to two key downsides of putting them all in the same directory

Both of these are valid.

There's also a parameter "generate_exe" which points to the actual executable. We could export everything to separate dirs and then symlink the executable itself into a bin dir

@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 7, 2024

Looks like a very sensible solution to me, and --bin seems fine to me. I would put them in different dirs, but the symlinking is a good idea so that it's easy to access them via a single PATH entry.

@lilatomic lilatomic force-pushed the feature/export-downloadable-tools branch 3 times, most recently from 651638f to b908951 Compare August 19, 2024 04:26
@lilatomic lilatomic force-pushed the feature/export-downloadable-tools branch from b908951 to 04cafbd Compare August 28, 2024 20:35
@lilatomic
Copy link
Contributor Author

do we want to make all TemplatedExternalTools exportable? We could extend Subsystem.rules and add the UnionRule there

@lilatomic lilatomic force-pushed the feature/export-downloadable-tools branch from 6b1f68d to 72636b6 Compare September 5, 2024 02:12
@huonw
Copy link
Contributor

huonw commented Sep 11, 2024

We've just branched for 2.23, so merging this pull request now will come out in 2.24, please move the release notes updates to docs/notes/2.24.x.md. Thank you!

@lilatomic lilatomic force-pushed the feature/export-downloadable-tools branch from 274a125 to b25ad88 Compare September 15, 2024 17:26
@lilatomic lilatomic changed the title RFC: Make ExternalTool exportable Make ExternalTool exportable Sep 15, 2024
allows using a name users would expect,
even if this is different from the tool's downloaded binary name and the tool's full name
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

I think this is really nice, thanks for working on it

docs/notes/2.24.x.md Outdated Show resolved Hide resolved
src/python/pants/core/goals/export.py Outdated Show resolved Hide resolved
Comment on lines 294 to 299
ln_bin = shutil.which("ln")
if not ln_bin:
# I think ln_bin missing won't happen, but the error message otherwise is "No such file or directory (os error 2)" which isn't helpful
raise RuntimeError(
"Could not locate `ln` bin to link exported binaries to the `bin` dir"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine (i.e. I don't think we need to block merging this, but could instead refactor later), but did you consider the option of creating a digest with symlinks in it and including that in the "main" workspace.write_digest(...) call? Rather than shell out to symlinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good point. I lifted the idea from the python backend's export implementation, which uses a PostProcessingCommand to symlink the venv in.
It probably makes more sense to collect all the symlinks and write them out, though. I'll see how hard that is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's a lot simpler, thanks for the suggestion! I've kept it as a separate write_digest to keep the locality of the processing, there's a lot going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also the symlinks are absolute symlinks, relative symlinks complained about the symlink not being in the subtree. Should be fine?

src/python/pants/core/goals/export.py Outdated Show resolved Hide resolved
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.

Support exporting downloaded external tools
3 participants