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

Use upstream coveragepy's new lcov support #14677

Closed

Commits on Feb 2, 2022

  1. rules/python: Rework built-in coveragepy support

    [Coveragepy recently gained support for lcov
    output][nedbat/coveragepy#1289], which allows implementing full
    support for python coverage without relying on a downstream fork of
    that project
    
    Coveragepy actually must be invoked twice; One generating a
    `.coverage` database file, the other exporting the data. This means
    that it is incompatible with the old implementation.
    
    The fork of coveragepy previously intended for use with bazel
    circumvented this by just changing how coveragepy works, and never
    outputting that database - just outputting the lcov directly
    instead. If we'd like to use upstream coveragepy, this is of course
    not possible.
    
    The stub_template seems to be written with the idea of supporting
    other coverage tooling in mind, however it still hard-codes arguments
    specific to coveragepy. Instead, we think it makes sense to properly
    support one of them for now, and to rethink a more generic interface
    later - it will probably take specific scripting for each
    implementation of coverage in python anyway.
    
    As such, this patch rewrites the python stub template to fully support
    upstream coveragepy as a coverage tool, and reworks some of the logic
    around invoking python to do so more cleanly.
    
    Additional notes:
    
      - Python coverage will only work with Python 3.7+ with upstream
        coveragepy, since the first release with lcov support does not
        support earlier Python versions - this is unfortunate, but there
        is not much we can do downstream short of forking to resolve
        that. The stub template itself should still work with Python 2.4+.
    
      - Comments in the code claim to use `os.execv` for performance
        reasons. There may be a small overhead to `subprocess.call`, but
        it shouldn't be too impactful, especially considering the overhead
        in logic (written in Python) this involves - if this is indeed for
        performance reasons, this is probably a somewhat premature
        optimization.
    
        A colleauge helped dig through some history, finding
        3bed4af as the source of this -
        if that commit is to believed, this is actually to resolve issues
        with signal handling, however that seems odd as well, since this
        calls arbitrary python applications, which in turn may use
        subprocesses again as well, and therefore break what that commit
        seems to attempt to fix.
    
        It's completely opaque to me why we put so much effort into trying
        to ensure we use `os.execv`. I've replicated the behavior and
        comments assuming it was correct previously, but the patch
        probably shouldn't land as-is - the comment explaining the use of
        `os.execv` is most likely misleading.
    
    ---
    
    [nedbat/coveragepy#1289]: nedbat/coveragepy#1289
    
    Co-authored-by: Bradley Burns <bradley.burns@codethink.co.uk>
    TLATER and bradb423 committed Feb 2, 2022
    Configuration menu
    Copy the full SHA
    72a0f9e View commit details
    Browse the repository at this point in the history
  2. WIP: rules/python: Add PYTHON_COVERAGE_TARGET

    This resolves bazelbuild#14436 by adding a new environment variable that will
    perform the coverage label resolution inside the python_stub_template
    directly, which resolves the python coverage tool by label rather than
    path.
    
    Currently this resolves the path in the runfiles directory by guessing
    the path the label should resolve to - this of course does not work in
    general, even just defining an alias breaks it. Since labels appear to
    only be resolved in the analysis phase, there does not seem to be an
    easy way around this, however.
    
    This is a draft, showing the behavior I would like - suggestions on
    how to correctly implement this would be appreciated.
    
    Co-authored-by: Bradley Burns <bradley.burns@codethink.co.uk>
    TLATER and bradb423 committed Feb 2, 2022
    Configuration menu
    Copy the full SHA
    f4f1c23 View commit details
    Browse the repository at this point in the history