Fix RuleRunner to work without BUILD_ROOT file in pytest sandbox

[cognifloyd]

After #20887 (pants 2.22.0.dev2), RuleRunner-based tests do not work in external repos: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1719344475216619

This test run shows the ValueErrors that show up in RuleRunner-based tests: https://github.com/StackStorm/st2/actions/runs/9667956659/job/26670955784

The tests were passing in the pants repo because rule_runner.py had an explicit dependency on //BUILD_ROOT:files. So, the BUILD_ROOT file was added to the pytest sandbox for every RuleRunner-based test. Removing the rule_runner.py dependency on //BUILD_ROOT:files causes the RuleRunner-based tests to fail in the pants repo as well.

This issue was exposed because the rust-based NativeOptionParser does not provide an API for overriding the BuildRoot, unlike the python layer which uses a singleton BuildRoot object and allows RuleRunner to replace the build_root, bypassing the sentinel file-lookup logic. The NativeOptionParser requires either the PANTS_BUILDROOT_OVERRIDE env var or a sentinel file (one of BUILD_ROOT, BUILDROOT, or pants.toml) in the current working directory (or a parent of the current working directory) to calculate the build_root.

This PR:

• Removes dependencies on //BUILD_ROOT:files for some tests that do not actually require it.
• Removes the explicit dependency on //BUILD_ROOT:files from testutil/rule_runner.py.
• Makes RuleRunner change the current working directory to the temporary build_root directory while bootstrapping options (ie for the codepaths that initialize the rust-based NativeOptionParser).
• Makes RuleRunner add the sentinel file in the temporary build_root directory that RuleRunner creates so NativeOptionParser can find it in the current working directory.
• Adds a RuleRunner.pushd() api method to allow tests to change the current directory to RuleRunner.build_root.
• Adds dependencies on //BUILD_ROOT:files for tests that relied on the transitive dep via testutil/rule_runner.py. These tests either did not use RuleRunner (eg they only used mock_console), or relied on working in the sandbox with the actual pants code under test (meaning the pytest sandbox IS the build_root for that test).

[cognifloyd]

I'm slowly working through all of the test failures to fix them by either using rule_runner.pushd() or adding the explicit dep on //BUILD_ROOT:files just to those tests. I run a few tests locally, but let CI run all of them to see what I missed.

[cognifloyd]

Tests are passing now.

@benjyw I would like your feedback on this before I merge it and cherry-pick it to 2.22.x.

[sureshjoshi]

High level, I don't have a good understanding of the originating problem but the changes themselves seem mostly mechanical. I see green checkmarks, so I'm fine with that.

The pushd API doesn't give me the fuzzies, but I don't really know why. I get that it works around the underlying problem, but I guess it makes me a bit nervous that we might write some tests that'll fail, and then we'll have to know/deduce to add that context manager. I guess my lack of understanding of the root cause means that I can't differentiate between a bug fix vs a bandaid. So, take my thoughts with the appropriate quantity of salt.

I'm kinda curious for @benjyw's thoughts on this, as I think he wrote the new native options parser - so I just want to ensure we're not working around something that should be handled at the Rust layer?

[cognifloyd]

High level, I don't have a good understanding of the originating problem but the changes themselves seem mostly mechanical. I see green checkmarks, so I'm fine with that.

:+1: The linked slack conversation has a bit more context. Working on this proved confusing for me too.

The pushd API doesn't give me the fuzzies, but I don't really know why. I get that it works around the underlying problem, but I guess it makes me a bit nervous that we might write some tests that'll fail, and then we'll have to know/deduce to add that context manager. I guess my lack of understanding of the root cause means that I can't differentiate between a bug fix vs a bandaid. So, take my thoughts with the appropriate quantity of salt.

I don't much care for it either. I tried (unsuccessfully) to encapsulate all of the wonky cwd management logic within the RuleRunner. I had to do it manually in some tests however, especially tests that create an options bootstrapper. At first I did with pushd(rule_runner.build_root): which felt rather verbose/repetitive, so I ended up encapsulating that in the rule_runner.pushd() method. It's still ugly, and a bit hand-wavy. But it's more explicit hand-waving than all of the tests that inadvertently depended on the BUILD_ROOT file magically being at the root of the pytest sandbox once they imported anything from pants.testutils.rule_runner.

I'm kinda curious for @benjyw's thoughts on this, as I think he wrote the new native options parser - so I just want to ensure we're not working around something that should be handled at the Rust layer?

I wonder about doing something in the rust layer too. It seems like we need a way for tests to tell the rust layer what the build_root should be, without requiring the sentinel file. The python layer has that feature, so adding it to the rust layer sounds reasonable.

But I'm not confident enough with rust to pull that off. I started in that direction, confused myself in the process, and then asked @benjyw which solution he preferred (rust api to override build_root detection or sentinel file in the sandbox) -- he said to add the sentinel file to the sandbox.

Adding the sentinel file ended up being somewhat convoluted as well. So, I'm not sold that this is better than something in the rust layer. But, it at least gets tests to fail like they do in external repos, and then patches up the fallout in the pants repo. So, it's probably good enough for a 2.22.x backport, which I think needs to happen before 2.22.0 final is released.

[kaos]

I agree with @sureshjoshi about this being mostly of the bandaid kind. The better approach I think would be to not rely on CWD as much as we do now (historically, it was assumed that CWD was always the same as the build root) -- this being the case have added some hoops to jump through for tests that exercises pants from a test sandbox, as then the CWD is not the build root. Those hoops are now starting to fall apart with the move of options into rust land, so this fix mends that by maintaining the assumption about the relationship of CWD with build root, rather than getting rid of the assumption (which I think would be the preferred solution long term, but is likely significantly more work to get done, so I'm 👍🏽 with this as a stop gap in the mean time).

My $.02 :)

[WorkerPants]

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

:heavy_check_mark: 2.22.x

Successfully opened https://github.com/pantsbuild/pants/pull/21121.

---

Thanks again for your contributions!

:robot: Beep Boop here's my run link