bazel
bazel copied to clipboard
bazelbuild
rules/python: Add a `coverage_tool` attribute to `py_runtime`.
This allows users to specify a target providing the coveragepy tool (and its dependencies). This is essential for hermetic python builds, where an absolute path will not really work. It's also superior to other potential methods using environment variables because the runfiles dependency on the coverage tool and its files is only incurred when building with coverage enabled.
This also builds on the work @TLATER began with https://github.com/bazelbuild/bazel/pull/14677 to integrate with coveragepy's lcov support, with an additional step of at least attempting to convert the absolute paths which coveragepy uses in the lcov output into the relative paths which the rest of bazel can actually consume.
This is my first time touching Java code professionally, so I'll admit to mostly cargo-culting those parts, and would welcome any feedback on how to improve things there. I also would have no objections to someone else taking over this PR to get it over the finish line. I've tested this out with our own team's internal monorepo, and have successfully generated a full combined coverage report for most of our python and go code. There's still a bunch of things which don't quite work, in particular when it comes to compiled extension modules or executables run from within python tests, but those will need to be addressed separately, and this is already a giant step forward for our team.
Closes https://github.com/bazelbuild/bazel/issues/14436.
Well, in its current state this works fine for our internal setup, which uses a hermetic python but with additional python packages installed (as required) into that python's search path, rather than some separate PYTHONPATH entry. Unfortunately when I tried setting up a test that would work the way most people are more likely to have things set up, it doesn't work so well:
- The
pip_installrule depends on having a configured python toolchain, which results in a circular dependency if you try to use it to fetch thecoveragepackage. - You can fetch coverage with e.g.
http_archive(
name = "coverage_linux_x64",
build_file_content = """
filegroup(
name = "coverage",
srcs = ["coverage/__main__.py"],
data = glob(["coverage/**"]),
visibility = ["//visibility:public"],
)
""",
sha256 = "84631e81dd053e8a0d4967cedab6db94345f1c36107c71698f746cb2636c63e3",
type = "zip",
urls = [
"https://files.pythonhosted.org/packages/74/0d/0f3c522312fd27c32e1abe2fb5c323b583a5c108daf2c26d6e8dfdd5a105/coverage-6.4.1-cp39-cp39-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl",
],
)
The problem with that being that you get
File "/.../my_test.runfiles/coverage_linux_x64/coverage/__main__.py", line 7, in <module>
from coverage.cmdline import main
ModuleNotFoundError: No module named 'coverage'
The obvious solution would be to check if the coverage_tool attribute has a PyInfo provider and, if so, all it to augment the PYTHONPATH (e.g. with the imports attribute on py_library). There are two problems with that approach:
- Ideally we wouldn't be adding stuff to the test's
PYTHONPATHfor this. But I suppose that's unavoidable. py_librarydoesn't have a way to declare an executable entry point. So we'd need to either declare a separate attribute onpy_runtimefor the entry point, or execute it aspython -m coverage .... The latter has some advantages, but also some significant disadvantages. If we prepend the PYTHONPATH forcoverage, we might cause unexpected behavior for the package under test, and if we append it to the end then we might not get thecoveragetool we expect (though, maybe packages which put a module namedcoverageinto the global namespace kind of deserve broken behavior).
Using py_binary for this would obviously be a different kind of problematic.
Never mind, the problem is a different one: it's not including the runfiles for coverage_tool, though it needs to.
Ok, with the addition of an integration test, I now feel like this is about ready to transition out of draft status. @TLATER would appreciate it if you have any further feedback to offer before I do that.
Well, the integration test passes locally for me. Honestly I don't actually know what the best practice would be for making it work on the builders where it's failing. I don't think people would be terribly happy about vendoring in an entire python runtime + coverage wheel (or really several, since if I was being less lazy about it, it would be including targets for macos and aarch64 as well), especially given that they're not even including a complete version of rules_python. Then again, maybe it would be a good idea just to have better coverage for the hermetic python use case. This would all be a lot simpler if py_runtime was defined in starlark rather than java so that this change could be made entirely in rules_python without directly impacting the main bazel repo, though the fact that the stub lives in the main bazel repo makes that challenging.
I'm not sure what the best way forward is in the mean time - I don't like the idea of proposing this change without a CI-passing test case.
I'll need to rebase this due to conflicts. I think once I'm doing that, I might actually split this PR into two, one to deal with getting pycoverage to actually produce useful results (building off of @TLATER 's change and adding in the bits for fixing up the paths, which we're going to continue to need until that part is fixed upstream) and a second change to add the attribute to py_runtime, since the later represents an API change, and for which it is apparently a bit tricky to write a test that will work on bazel's CI infrastructure, which currently seems to have little to no testing for properly hermetic python toolchains. However I'm on PTO this week so it'll have to wait.
Hello @adam-azarchs , Can you please check above build failures and Code conflict. Thank you!
Yes, sorry I just haven't had a chance to circle back around to this. I'll fix the conflicts and then see what I can do about the tests.
As was recommended, I updated the unit test to use a mock coverage script. After fixing various issues with other platforms, tests are now passing. I can clean up the commit log if anyone wishes me to do so.
I'll have a look. (coincidentally, we made essentially this same change internally a couple months ago, but since we don't use toolchains and do embedded hermetic python, our impl looks entirely different; same basic problems encountered and solved though)
Test failure looks to be caused by efa196c7857945f4774a8930e6fd63c9bad698e6, unrelated to my changes. Once master is passing again I'll merge that in.
Friendly ping! I would love for our internal CI to be able to start using this feature in official weekly builds some time in the not so distant future, rather than resorting to building off of my dev branch.
I'm not familiar with the Python rules or how coverage works for them, but with that caveat, LGTM. ;)
Oops, accidentally pushed a button on the github UI that made it merge from master. Not that that's a bad thing exactly, but I'm going to assume that that clobbers the approvals.
Related note, I'm not sure how stuff gets from having the awaiting-PR-merge label to actually being merged?
It says "changed approved" still, so I think the approval is still good. Thanks for bumping the thread -- if something lingers for more than a day or so please tag me.
@c-mita Can you please merge? I don't have permission to do so.
Oh, wait, I think I see now. "After your review is complete, a Bazel maintainer applies your patch to Google's internal version control system."
I'll see if I can figure out how to do that internally.
Related note, I'm not sure how stuff gets from having the awaiting-PR-merge label to actually being merged?
A member of the Bazel support team will import it into Google. This label is constantly monitored by them. @sgowroji just in case this fell off your radar, could you import this please? Thanks :)
