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.

Sign up for GitHub

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

Jump to bottom

Fix RuleRunner to work without BUILD_ROOT file in pytest sandbox #21112

Merged
merged 18 commits into from
Jul 1, 2024
Merged

Fix RuleRunner to work without BUILD_ROOT file in pytest sandbox #21112

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
613fc8a
minimize dependencies on //BUILD_ROOT
cognifloyd Jun 27, 2024
6ade39c
Remove //BUILD_ROOT dependency from RuleRunner
cognifloyd Jun 27, 2024
4388e1d
fmt with black
cognifloyd Jun 27, 2024
a162159
Adjust cwd in more places where RuleRunner indirectly creates NativeO…
cognifloyd Jun 27, 2024
46189c9
Adjust cwd in more places where RuleRunner indirectly creates NativeO…
cognifloyd Jun 27, 2024
bdf1d32
add RuleRunner.pushd method to cd to build_root
cognifloyd Jun 28, 2024
a54a5ac
Drop import-time side-effects (calculation of build_root)
cognifloyd Jun 28, 2024
081def4
reuse rule_runner.options_bootstrapper to avoid recalculation of buil…
cognifloyd Jun 28, 2024
8acc1ef
add test-specific deps on //BUILD_ROOT where required
cognifloyd Jun 28, 2024
4a6fb0b
fmt
cognifloyd Jun 28, 2024
38a3418
deal with BUILDROOT file for more tests
cognifloyd Jun 28, 2024
87dbbcd
fmt
cognifloyd Jun 28, 2024
3a23a89
fix cwd for bsp+rule_runner tests
cognifloyd Jun 28, 2024
032c3f8
Merge branch 'main' into cognifloyd/rulerunner-buildroot
cognifloyd Jun 28, 2024
a938ca4
fmt
cognifloyd Jun 28, 2024
725e057
add merge conflict magnet
cognifloyd Jun 28, 2024
a84d29b
pants_explorer: use module level SpecsParser, but delay init
cognifloyd Jun 30, 2024
37d4ef5
add an example to RuleRunner note in docs/notes
cognifloyd Jun 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/python/pants/base/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ python_sources()
python_tests(
name="tests",
sources=["*_test.py", "!exception_sink_test.py", "!*_integration_test.py"],
dependencies=["//BUILD_ROOT:files"],
overrides={
"build_root_test.py": dict(
dependencies=["//BUILD_ROOT:files"],
)
},
)

python_tests(
Expand Down
1 change: 0 additions & 1 deletion src/python/pants/testutil/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ python_sources(
"pants_integration_test.py": {
"dependencies": ["//BUILD_ROOT:files", "src/python/pants/__main__.py"]
Copy link
Member Author

@cognifloyd cognifloyd Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I also tried removing the //BUILD_ROOT:files dependency from pants_integration_test.py. But that didn't work, and it turned out to be a more invasive change.

Fixing this will require removing or changing tmpdir creation functions in this file that call get_buildroot() repeatedly. We need to change that assumption so the test author can pass in the build root instead of having these functions calculate it from the current working directory, which happens to be the pytest sandbox.

I suspect this issue existed before #20887 (ie it is not a new bug in 2.22.x), but pants_integration_test-based tests are far less common than RuleRunner based tests. Since this is (probably) not a new bug, we can deal with changing the assumptions in this code at some later date.

},
"rule_runner.py": {"dependencies": ["//BUILD_ROOT:files"]},
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix that makes the errors in external repos visible in the pants repo.

},
)

Expand Down
72 changes: 42 additions & 30 deletions src/python/pants/testutil/rule_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
overload,
)

from pants.base.build_environment import get_buildroot
from pants.base.build_root import BuildRoot
from pants.base.specs_parser import SpecsParser
from pants.build_graph.build_configuration import BuildConfiguration
Expand Down Expand Up @@ -310,17 +311,23 @@ def rewrite_rule_for_inherent_environment(rule):
self.build_config = build_config_builder.create()

self.environment = CompleteEnvironmentVars({})
self.options_bootstrapper = self.create_options_bootstrapper(args=bootstrap_args, env=None)
self.extra_session_values = extra_session_values or {}
self.inherent_environment = inherent_environment
self.max_workunit_verbosity = max_workunit_verbosity
options = self.options_bootstrapper.full_options(
self.build_config,
union_membership=UnionMembership.from_rules(
rule for rule in self.rules if isinstance(rule, UnionRule)
),
)
global_options = self.options_bootstrapper.bootstrap_options.for_global_scope()

# Change cwd and add sentinel file (BUILDROOT) so NativeOptionParser can find build_root.
with pushd(self.build_root):
Path("BUILDROOT").touch()
Copy link
Member Author

Choose a reason for hiding this comment

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

I opted to use BUILDROOT instead of BUILD_ROOT so that anyone who needs to inspect a sandbox has a better idea where the file comes from (or can more easily grep for where it comes from).

Copy link
Member

Choose a reason for hiding this comment

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

I didn't fully understand this comment. I'm fine with either one, but did you mean, using BUILDROOT in lieu of BUILD_ROOT so that it lines up with the other usages?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a BUILD_ROOT file at the root of the pants repo. Some tests depend on that (//BUILD_ROOT:files), so it gets copied to the root of the pytest sandbox.

Now, RuleRunner also adds a sentinel file to its own sandboxes. Using the variant with _ gives a subtle indicator of why the file is in the sandbox / what created the file; It gives a subtle indicator that the file is not a copy of the file that //:BUILD_ROOT:files pulls in.

self.options_bootstrapper = self.create_options_bootstrapper(
args=bootstrap_args, env=None
)
options = self.options_bootstrapper.full_options(
self.build_config,
union_membership=UnionMembership.from_rules(
rule for rule in self.rules if isinstance(rule, UnionRule)
),
)
global_options = self.options_bootstrapper.bootstrap_options.for_global_scope()

dynamic_remote_options, _ = DynamicRemoteOptions.from_options(options, self.environment)
local_store_options = LocalStoreOptions.from_options(global_options)
Expand Down Expand Up @@ -398,7 +405,8 @@ def request(self, output_type: type[_O], inputs: Iterable[Any]) -> _O:
if self.inherent_environment
else Params(*inputs)
)
result = assert_single_element(self.scheduler.product_request(output_type, [params]))
with pushd(self.build_root):
result = assert_single_element(self.scheduler.product_request(output_type, [params]))
return cast(_O, result)

def run_goal_rule(
Expand All @@ -413,26 +421,28 @@ def run_goal_rule(
merged_args = (*(global_args or []), goal.name, *(args or []))
self.set_options(merged_args, env=env, env_inherit=env_inherit)

raw_specs = self.options_bootstrapper.full_options_for_scopes(
[GlobalOptions.get_scope_info(), goal.subsystem_cls.get_scope_info()],
self.union_membership,
).specs
with pushd(self.build_root):
raw_specs = self.options_bootstrapper.full_options_for_scopes(
[GlobalOptions.get_scope_info(), goal.subsystem_cls.get_scope_info()],
self.union_membership,
).specs
specs = SpecsParser(root_dir=self.build_root).parse_specs(
raw_specs, description_of_origin="RuleRunner.run_goal_rule()"
)

stdout, stderr = StringIO(), StringIO()
console = Console(stdout=stdout, stderr=stderr, use_colors=False, session=self.scheduler)

exit_code = self.scheduler.run_goal_rule(
goal,
Params(
specs,
console,
Workspace(self.scheduler),
*([self.inherent_environment] if self.inherent_environment else []),
),
)
with pushd(self.build_root):
exit_code = self.scheduler.run_goal_rule(
goal,
Params(
specs,
console,
Workspace(self.scheduler),
*([self.inherent_environment] if self.inherent_environment else []),
),
)

console.flush()
return GoalRuleResult(exit_code, stdout.getvalue(), stderr.getvalue())
Expand Down Expand Up @@ -465,7 +475,8 @@ def set_options(
**{k: os.environ[k] for k in (env_inherit or set()) if k in os.environ},
**(env or {}),
}
self.options_bootstrapper = self.create_options_bootstrapper(args=args, env=env)
with pushd(self.build_root):
self.options_bootstrapper = self.create_options_bootstrapper(args=args, env=env)
self.environment = CompleteEnvironmentVars(env)
self._set_new_session(self.scheduler.scheduler)

Expand Down Expand Up @@ -778,14 +789,15 @@ def mock_console(
*,
stdin_content: bytes | str | None = None,
) -> Iterator[tuple[Console, StdioReader]]:
global_bootstrap_options = options_bootstrapper.bootstrap_options.for_global_scope()
colors = (
options_bootstrapper.full_options_for_scopes(
[GlobalOptions.get_scope_info()], UnionMembership({}), allow_unknown_options=True
with pushd(get_buildroot()):
global_bootstrap_options = options_bootstrapper.bootstrap_options.for_global_scope()
colors = (
options_bootstrapper.full_options_for_scopes(
[GlobalOptions.get_scope_info()], UnionMembership({}), allow_unknown_options=True
)
.for_global_scope()
.colors
)
.for_global_scope()
.colors
)

with initialize_stdio(global_bootstrap_options), stdin_context(
stdin_content
Expand Down
Loading