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
Locate bash before using it to locate BinaryPaths for other tools #10858
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,18 +10,29 @@ | |
from pants.engine.fs import CreateDigest, Digest, DigestContents, FileContent, PathGlobs, Snapshot | ||
from pants.engine.internals.scheduler import ExecutionError | ||
from pants.engine.process import ( | ||
BinaryPathRequest, | ||
BinaryPaths, | ||
FallibleProcessResult, | ||
InteractiveProcess, | ||
Process, | ||
ProcessExecutionFailure, | ||
ProcessResult, | ||
) | ||
from pants.engine.rules import Get, rule | ||
from pants.testutil.rule_runner import QueryRule | ||
from pants.testutil.pants_integration_test import setup_tmpdir | ||
from pants.testutil.rule_runner import QueryRule, RuleRunner | ||
from pants.testutil.test_base import TestBase | ||
from pants.util.contextutil import temporary_dir | ||
|
||
|
||
def process_rule_runner() -> RuleRunner: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be annotated with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, if we expect multiple rule runners in the file with different setups, you can call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I'm not a huge fan of the fixtures. I don't see a great reason to do:
rather than:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's totally valid to create a
Conformity with the greater Python ecosystem, which makes heavy use of fixtures. Our Plugin API is now aligned with the conventions. We also use pre-built fixtures in other places too now like Finally, it's important that plugin authors don't try using one rule runner for multiple tests, such as a class var or global consant. The fixture is meant to emphasize that you want a new instance every test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unless you accidentally put I don't feel strongly about it. But particularly if we're discouraging the reuse of fixtures, they don't seem to buy much. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto, I don't care that much either way, so long as we're consistent in plugin docs and we feel confident that users won't accidentally create one instance for >1 test. I'm curious what John and Benjy think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I switched it so that we're consistent for now. |
||
return RuleRunner( | ||
rules=[ | ||
QueryRule(BinaryPaths, [BinaryPathRequest]), | ||
], | ||
) | ||
|
||
|
||
@dataclass(frozen=True) | ||
class Concatted: | ||
value: str | ||
|
@@ -442,3 +453,18 @@ def test_running_interactive_process_in_workspace_cannot_have_input_files() -> N | |
mock_digest = Digest("fake", 1) | ||
with pytest.raises(ValueError): | ||
InteractiveProcess(argv=["/bin/echo"], input_digest=mock_digest, run_in_workspace=True) | ||
|
||
|
||
def test_find_binary_on_path_without_bash() -> None: | ||
stuhood marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Test that locating a binary on a PATH which does not include bash works (by recursing to | ||
# locate bash first). | ||
binary_name = "mybin" | ||
binary_dir = "bin" | ||
with setup_tmpdir({f"{binary_dir}/{binary_name}": "this just needs to exist"}) as tmpdir: | ||
binary_dir_abs = os.path.join(os.getcwd(), tmpdir, binary_dir) | ||
search_path = [binary_dir_abs] | ||
binary_paths = process_rule_runner().request( | ||
BinaryPaths, [BinaryPathRequest(binary_name=binary_name, search_path=search_path)] | ||
) | ||
assert binary_paths.first_path is not None | ||
assert binary_paths.first_path.path == os.path.join(binary_dir_abs, binary_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please update
archive.py
to use this value, rather than duplicating? Will keep the surface area smaller.Maybe rename to something more generic, although I can't think of a good name for the binaries that we expect to be installed in
/bin
et al.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
archive.py
search path arguably should be configurable (as commented there), since it's used for multiple tools. This one on the other hand does not seem like it should be, necessarily, as we're using it to bootstrap.