search

pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.29k stars 628 forks source link

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

Closed cognifloyd closed 3 months ago

cognifloyd commented 3 months ago

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:

cognifloyd commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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