Locate bash before using it to locate BinaryPaths for other tools #10858
Conversation
[ci skip-rust] [ci skip-build-wheels]
stuhood
changed the title
| 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.
This should be annotated with @pytest.fixture to ensure that each unit test gets a fresh instance. Also we've been calling it rule_runner for consistency.
There was a problem hiding this comment.
Alternatively, if we expect multiple rule runners in the file with different setups, you can call it find_binary_rule_runner.
There was a problem hiding this comment.
So, I'm not a huge fan of the fixtures. I don't see a great reason to do:
def test_my_stuff(rule_runner: RuleRunner) -> None:
..
rather than:
def test_my_stuff() -> None:
rule_runner = my_rule_runner()
There was a problem hiding this comment.
I think it's totally valid to create a RuleRunner() inline. I do that in some places. But imo, it should either be inline, or it should be a fixture. Right now is a mix of having the function declared as top-level.
So, I'm not a huge fan of the fixtures. I don't see a great reason
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 caplog, and I think it's good to be consistent with those.
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.
The fixture is meant to emphasize that you want a new instance every test.
Unless you accidentally put @pytest.fixture(scope=module) on it, which would get you a reused one.
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.
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.
I switched it so that we're consistent for now.
| @@ -30,6 +30,9 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| BASH_SEARCH_PATHS = ("/usr/bin", "/bin", "/usr/local/bin") | |||
There was a problem hiding this comment.
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.
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.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Problem
The first error in #10855 is that when we search a search path which does not include
bash, lookups fail silently because the discovery script fails to start. The usecase described on #9760 (settinginterpreter_search_path=["<PYENV>"]) is one example wherebashwill not be present.Solution
Recurse to locate an absolute path for
bashbefore usingbash. Additionally, change the "discovery" portion ofBinaryPathsfromFallibleProcessResulttoProcessResultto fail more quickly in cases where discovery errors, as opposed to succeeding with an empty result.Result
The added test passes, and the usecase from #9760 works on my machine.
[ci skip-rust]
[ci skip-build-wheels]