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

More flexible binary and run rules. #9529

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Apr 13, 2020

Kill coordinator_of_binaries and implement binary and run validation
separately.

The binary goal now supports both globbing that includes targets that do not
produce a binary as well as production of multiple binaries per target.

The run goal now also supports globbing as long as exactly one of the globbed
targets can produce a binary.

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

@jsirois
Copy link
Contributor Author

jsirois commented Apr 13, 2020

I was working on an experiment that used two binary targets in the same directory and I went to build them with ./v2 binary src/python/pants/backend/python/rules/pex_tools: and got a blow-up since the directory also contained a common library the binaries used. It was frustrating to not be able to say "build all my binaries" in a ~sane way like I could in v1.

With this change the ergonomics of binary and run change - I think for the better:

Before:

$ ./v2 --no-print-exception-stacktrace binary src/python/pants::
...
ERROR: The `run` and `binary` goals only work with the following target types: ['python_binary']

You used src/python/pants/backend/native/subsystems:subsystems with target type 'python_library'.

After:

$ ./v2 --no-print-exception-stacktrace binary src/python/pants::

Wrote dist/builder-bin.pex
Wrote dist/pants.pex
Wrote dist/pants_local_binary.pex
Wrote dist/resolver-bin.pex
Wrote dist/reversion.pex
Wrote dist/s3_log_aggregator.pex

Before:

$ ./v2 --no-print-exception-stacktrace binary src/python/pants/base:
...
ERROR: The `run` and `binary` goals only work with the following target types: ['python_binary']

You used src/python/pants/base:build_root with target type 'python_library'.

After:

$ ./v2 --no-print-exception-stacktrace binary src/python/pants/base:
...
ERROR: The `binary` goal only works with the following target types:
  * python_binary

You specified `src/python/pants/base:` which only included the following target types:
  * python_library
  * python_tests

The run goal now also supports globbing as long as exactly one of the
globbed targets can produce a binary.

Before:

$ ./v2 --no-print-exception-stacktrace run src/python/pants/util:
...
ERROR: Expected a single target, but was given multiple targets.

Did you mean one of:
  * src/python/pants/util:argutil
  ...
  * src/python/pants/util:xml_parser

After:

$ ./v2 --no-print-exception-stacktrace run src/python/pants/util:


Paths by count:
===============

Paths by bytes:
===============

IPs by count:
=============

IPs by bytes:
=============


Running target: src/python/pants/util:s3_log_aggregator
src/python/pants/util:s3_log_aggregator ran successfully.

Before:

$ ./v2 --no-print-exception-stacktrace run src/python/pants/bin:
...
ERROR: Expected a single target, but was given multiple targets.

Did you mean one of:
  * src/python/pants/bin:bin
  * src/python/pants/bin:pants
  * src/python/pants/bin:pants_local_binary

After:

$ ./v2 --no-print-exception-stacktrace run src/python/pants/bin:
...
ERROR: The `run` goal only works on one binary target but was given multiple targets that can produce a binary:
  * src/python/pants/bin:pants
  * src/python/pants/bin:pants_local_binary

Please select one of these targets to run.

Before:

$ ./v2 --no-print-exception-stacktrace run src/python/pants/base:
...
ERROR: Expected a single target, but was given multiple targets.

Did you mean one of:
  * src/python/pants/base:build_environment
  ...
  * src/python/pants/base:workunit

After:

$ ./v2 --no-print-exception-stacktrace run src/python/pants/base:
...
ERROR: The `run` goal only works with the following target types:
  * python_binary

You specified `src/python/pants/base:` which only included the following target types:
  * python_library
  * python_tests

@jsirois
Copy link
Contributor Author

jsirois commented Apr 13, 2020

Note to self: update with #9528 for improved run_test.

@Eric-Arellano Eric-Arellano requested a review from benjyw April 13, 2020 17:30
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks. This is a nice UX win.

) -> Run:
address = addresses.expect_single()
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, neat. Sounds good to me to support globbing.

In a followup, we will likely want to add this flexibility to test --debug:

if options.values.debug:
target_with_origin = targets_with_origins.expect_single()

And we should consider removing the expect_single() methods entirely.

@jsirois jsirois force-pushed the binary_and_run/more_flexible branch from 191d850 to bde9b80 Compare April 14, 2020 04:18
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This looks great.

Thank you for only factoring one level higher and not more than that - I'm not yet sure how we'll want to generalize all this Config code and want to finish porting lint, repl, and setup-py first.

union_membership: UnionMembership,
registered_target_types: RegisteredTargetTypes,
) -> Binary:
valid_config_types_by_target = gather_valid_binary_configuration_types(
Copy link
Contributor

Choose a reason for hiding this comment

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

I love how this rule reads now. Wonderful.

jsirois added 2 commits April 14, 2020 05:01
Kill coordinator_of_binaries and implement binary and run validation
separately.

The `binary` goal now supports both globbing that includes targets that do not
produce a binary as well as production of multiple binaries per target.

The `run` goal now also supports globbing as long as exactly one of the globbed
targets can produce a binary.
[ci skip-rust-tests]
[ci skip-jvm-tests]
This cenralizes complex logic for binary and run to share.
Error messages are made a bit easier to read by riffling lists as well.
[ci skip-jvm-tests]
@jsirois jsirois force-pushed the binary_and_run/more_flexible branch from bde9b80 to c26372c Compare April 14, 2020 12:01
@jsirois jsirois merged commit 84f8b8c into pantsbuild:master Apr 14, 2020
@jsirois jsirois deleted the binary_and_run/more_flexible branch April 14, 2020 21:12
Eric-Arellano added a commit that referenced this pull request Apr 21, 2020
## Problem

#9529 introduced the following flexible behavior for `binary` and `run`:

* Both `run` and `binary` will error if no valid targets were given.
* `run` will error if more than one _valid_ target was given. It doesn't care if there are invalid targets.
* `binary` will filter out all irrelevant targets.

The PR intentionally did not generalize the functionality because of the provisional status of the Configuration code. So, `test --debug` did not benefit and still would error if given more than one address, regardless of the address is irrelevant. `awslambda` also was duplicating the functionality of `binary` in a worse way.

## Solution

This PR generalizes the pattern and uses it with `awslambda`, `binary`, `run`, and `test`. We do not use it with `fmt`, `lint`, and `repl` because they all interact with the Target API differently.

## Result

`./v2 test --debug src/python/pants/util::` now works.

Invalid targets give nicer messages for `awslambda` and `test`:

```
▶ ./v2 --no-print-exception-stacktrace test 'src/python/pants/util/*util.py'


ERROR: The `test` goal only works with the following target types:
  * python_tests

You specified `src/python/pants/util/*util.py`, which only included the following target types:
  * files
  * python_library
```
[ci skip-rust-tests]
[ci skip-jvm-tests]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants