[kcov] Support python coverage with Bazel 6 #18616
Conversation
There was a problem hiding this comment.
Closes #18615.
\cc @williamjallen FYI. No action required.
+@jwnimmer-tri for feature review.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @jwnimmer-tri)
|
@drake-jenkins-bot linux-focal-gcc-bazel-experimental-coverage please |
|
Needs to be tested with some coverage builds. @drake-jenkins-bot linux-focal-clang-bazel-experimental-coverage please. |
There was a problem hiding this comment.
jinx
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @jwnimmer-tri)
jwnimmer-tri
added
status: single reviewer ok
There was a problem hiding this comment.
@jwnimmer-tri: lgty?
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)
|
Still working its way up my stack. I haven't tested locally yet. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @rpoyner-tri)
tools/dynamic_analysis/kcoverage.py line 22 at r1 (raw file):
parser.add_argument('verb', type=str, help='coverage.py verb') parser.add_argument('-a', '--append', action='store_true', default=False, help='coverage.py --append')
nit These help strings seem a bit of a pain to maintain, and I am not sure that any of these help strings is that helpful, either to someone reading this source code, or a user who accidentally ended up with --help output from this on the command line somehow.
Another tactic would be to define a local ...
ignore = "Drake will ignore this coverage.py argument."... and then do help=ignore throughout.
tools/dynamic_analysis/kcoverage.py line 26 at r1 (raw file):
help='coverage.py --branch') parser.add_argument('--rcfile', type=str, help='coverage.py rcfile') parser.add_argument('-o', type=str, help='coverage.py outfile')
I don't see -o in the latest coverage run --help? Ah, this is a coverage lcov option that we need to throw away. That took me a little digging to unearth.
What do you think about this alternative:
In main(), check for if sys.argv[1] != 'run' immediately, without using argparse yet. That way we can bail out for lcov immediately, and we don't need to throw away -o anymore or keep chasing any new options that non-run command lines might grow in the future.
That would also mean there's no need to return the argparse namespace object anymore; just the extras are enough.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions (waiting on @rpoyner-tri)
tools/dynamic_analysis/kcoverage.py line 54 at r1 (raw file):
args, extra = parse_command_line(sys.argv[1:]) if args.verb != 'run': return
Thinking about this line a bit more, I wonder if we should err a little bit more on the side of fail-fast here?
If the first argument is lcov, then we early-return declaring victory. (Possibly the same for other coverage verbs we know to be safely ignoreable.)
If the first argument is run then we run.
But if it is something else (e.g., if argument-passing or whitespace got screwed up somewhere along the line), we could fail-fast.
To work with the updated python stubs, pre-create our test output directory, and absorb/ignore more kinds of coverage-py command lines.
rpoyner-tri
force-pushed
the
kcov-bazel6
branch
from
1daf2e8 to
4c0021d
Compare
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-focal-clang-bazel-experimental-coverage please. |
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions (waiting on @jwnimmer-tri and @rpoyner-tri)
tools/dynamic_analysis/kcoverage.py line 26 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I don't see
-oin the latestcoverage run --help? Ah, this is acoverage lcovoption that we need to throw away. That took me a little digging to unearth.What do you think about this alternative:
In
main(), check forif sys.argv[1] != 'run'immediately, without using argparse yet. That way we can bail out forlcovimmediately, and we don't need to throw away-oanymore or keep chasing any new options that non-runcommand lines might grow in the future.That would also mean there's no need to return the argparse namespace object anymore; just the extras are enough.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @rpoyner-tri)
* [kcov] Support python coverage with Bazel 6 To work with the updated python stubs, pre-create our test output directory, and absorb/ignore more kinds of coverage-py command lines.
* [kcov] Support python coverage with Bazel 6 To work with the updated python stubs, pre-create our test output directory, and absorb/ignore more kinds of coverage-py command lines.
* [kcov] Support python coverage with Bazel 6 To work with the updated python stubs, pre-create our test output directory, and absorb/ignore more kinds of coverage-py command lines.
* [kcov] Support python coverage with Bazel 6 To work with the updated python stubs, pre-create our test output directory, and absorb/ignore more kinds of coverage-py command lines.
To work with the updated python stubs, pre-create our test output directory, and absorb/ignore more kinds of coverage-py command lines.
Closes #18615.
This change is