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

Extracts backend.adhoc package and backend.experimental.adhoc backend for adhoc_tool #18237

Merged
merged 21 commits into from
Feb 15, 2023

Conversation

chrisjrn
Copy link
Contributor

Per discussion in #18082, this extracts the target formerly known as experimental_run_in_sandbox, which is newly named adhoc_tool, into a new package at pants.backend.adhoc.

Registration for adhoc_tool is in pants.backend.experimental.adhoc, to reflect the non-stable nature of this target type.

adhoc_process_support.py has been moved to core.util_rules: to facilitate this, this file no longer depends on field/target types from the shell or adhoc backends. This makes for some slightly duplicative code in adhoc_tool.py and shell_command.py, which should become simpler after we branch for 2.16 (there's some deprecations in there).

With this change, we would be in a position to stabilise shell_command as-is.

@chrisjrn chrisjrn added category:user api change backend: Shell Shell backend-related issues labels Feb 13, 2023
@chrisjrn
Copy link
Contributor Author

It's definitely worth inspecting commits individually, there's a lot of moving parts, particularly in tests and shell_command.py

@chrisjrn chrisjrn marked this pull request as ready for review February 13, 2023 21:00
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

A first pass of things I've noticed. I haven't reviewed everything yet.

src/python/pants/bin/BUILD Outdated Show resolved Hide resolved
src/python/pants/backend/adhoc/target_types.py Outdated Show resolved Hide resolved
src/python/pants/backend/adhoc/target_types.py Outdated Show resolved Hide resolved
src/python/pants/backend/adhoc/target_types.py Outdated Show resolved Hide resolved
src/python/pants/backend/adhoc/target_types.py Outdated Show resolved Hide resolved
src/python/pants/backend/adhoc/target_types.py Outdated Show resolved Hide resolved
Comment on lines +65 to +66
execution_dependencies: tuple[str, ...] | None
dependencies: tuple[str, ...] | None # can go away after 2.17.0.dev0 per deprecation
Copy link
Member

Choose a reason for hiding this comment

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

Should these be the fields instead of the field values?

That would allow using request.execution_dependencies.alias in the _descr string below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal here was to make adhoc_process_support not depend on any custom field types. Happy to discuss the stylistic merits of it, but you're right, not being able to use .alias is a downside here.

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 another request type that will go away when we support scoped dependencies; sigh :))

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Good stuff!

@chrisjrn chrisjrn merged commit ac9e27b into pantsbuild:main Feb 15, 2023
@chrisjrn chrisjrn deleted the chrisjrn/adhoc-tool-backend branch February 15, 2023 17:55
seve-martinez pushed a commit to seve-martinez/pants that referenced this pull request Feb 20, 2023
…kend for `adhoc_tool` (pantsbuild#18237)

Per discussion in pantsbuild#18082, this extracts the target formerly known as
`experimental_run_in_sandbox`, which is newly named `adhoc_tool`, into a
new package at `pants.backend.adhoc`.

Registration for `adhoc_tool` is in `pants.backend.experimental.adhoc`,
to reflect the non-stable nature of this target type.

`adhoc_process_support.py` has been moved to `core.util_rules`: to
facilitate this, this file no longer depends on field/target types from
the `shell` or `adhoc` backends. This makes for some slightly
duplicative code in `adhoc_tool.py` and `shell_command.py`, which should
become simpler after we branch for 2.16 (there's some deprecations in
there).

With this change, we would be in a position to stabilise `shell_command`
as-is.

---------

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
{AdhocToolArgumentsField.alias}=[""],
{AdhocToolExecutionDependenciesField.alias}=[":scripts"],
{AdhocToolOutputDirectoriesField.alias}=["logs/my-script.log"],
{AdhocToolOutputFilesField.alias}=["results/"],
Copy link
Member

Choose a reason for hiding this comment

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

When looking at the rendered docs it was much easier to spot that the example field values here are swapped (files vs directories). 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Shell Shell backend-related issues category:user api change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants