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.

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

WIP: Add python+coverage example #46

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TLATER
Copy link

@TLATER TLATER commented Nov 11, 2022

Currently WIP because:

  • $(location) and $(rootpath) make vars aren't working correctly
    • This is what we tried to fix with the
      PYTHON_COVERAGE_TARGET variable, which was ultimately not accepted. See also next point.
  • Does not use the new toolchain feature that supports adding coveragepy directly to the python toolchain
    • Added together with the new coveragepy support in rules/python: Add a coverage_tool attribute to py_runtime. bazelbuild/bazel#15590, and is probably better to use than the $(rootpath) and pip_install solution we're showing here
    • While I'd love to rewrite the blog post to show this instead, I'd need a bit more time for that (at least a few hours). I'll see what Richard says, but I'm unsure I can make time for this right now.
  • Python imports are wonky
    • For some reason, the sibling module can only be imported under a parent python module. I confirmed this by looking through PYTHON_PATH
    • Seems to be a Bazel regression? Strangely nobody is complaining about this yet, we should probably raise an issue
  • Not tested in remote execution
    • No access to an engflow setup to test this right this instant
    • Since we're not using a toolchain (and even if we were we're using pip_install), this requires a python on the host, so there's a decent chance it won't work
  • Requires bumping Bazel to a version with rules/python: Add a coverage_tool attribute to py_runtime. bazelbuild/bazel#15590 merged, which currently is only available in prereleases

Currently WIP because:

- `$(location)` and `$(rootpath)` make vars aren't working correctly
  - This is what we tried to fix with the `PYTHON_COVERAGE_TARGET`
    variable, which was ultimately not accepted. See also next point.
- Does not use the new toolchain feature that supports adding
  coveragepy directly to the python toolchain
  - Added together with the new coveragepy support in
    bazelbuild/bazel#15590, and is probably better to use than the
    `$(rootpath)` and `pip_install` solution we're showing here
  - While I'd love to rewrite the blog post to show this instead, I'd
    need a bit more time for that
- Python imports are wonky
  - For some reason, the sibling module can only be imported under a
    parent `python` module. I confirmed this by looking through
    `PYTHON_PATH`
  - Seems to be a Bazel regression? Strangely nobody is complaining
    about this yet, we should probably raise an issue
- Not tested in remote execution
  - No access to an engflow setup to test this right this instant
  - Since we're not using a toolchain (and even if we were we're using
    `pip_install`), this requires a python on the host, so there's a
    decent chance it won't work
- Requires bumping Bazel to a version with bazelbuild/bazel#15590
  merged, which currently is only available in prereleases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant