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

complain if kcov_tool fails #288

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

complain if kcov_tool fails #288

wants to merge 2 commits into from

Conversation

rpoyner-tri
Copy link
Contributor

@rpoyner-tri rpoyner-tri commented Aug 29, 2024

This change is Reviewable

@rpoyner-tri rpoyner-tri force-pushed the kcov-error-checks branch 5 times, most recently from be64d71 to 160fcee Compare August 29, 2024 23:09
Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be keeping this in draft form for a bit. it turns out that kcov has been failing for two reasons: (1) kcov_tool bugs fixed in RobotLocomotion/drake#21859, and (2) bazel_tools implicitly depends on zip, and accidentally deletes coverage data if zip is absent.

So: I will add zip to Drake dependencies, then ask for updated images, and then come back to polishing these error checks.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request for new images is here: RobotLocomotion/drake#21865

Reviewable status: 0 of 1 files reviewed, all discussions resolved

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, bazelbuild/bazel#23479 is also now fixed and slated for bazel 7.4.0, but we still should have images that contain zip.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @rpoyner-tri)


driver/configurations/bazel/step-build.cmake line 59 at r1 (raw file):

else()
  set(BUILD_ARGS
    "${DASHBOARD_BAZEL_STARTUP_OPTIONS} test ${DASHBOARD_BAZEL_BUILD_OPTIONS} ${DASHBOARD_BAZEL_TEST_OPTIONS} //common:drake_assert_test //common:ssize_test")

This change will get reverted before merge. It's just here to make test cycles go faster.

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image updates are done, but the Jammy nodes are still running the old one. Will wait until they are updated, and then run an end-to-end test before trying to merge.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things are in good shape now. For examples of success and failure with this changes, see these build logs:

At r2, I've reverted the build target changes that made the above test builds conveniently short.

+@BetsyMcPhail for review.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @BetsyMcPhail)

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as non-zero exit codes from kcov_tool get noticed, I'm happy. The scope of the changes in r3 probably wants a more cmake-and-cdash-literate reviewer, maybe @mwoehlke-kitware ?

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @BetsyMcPhail)

BetsyMcPhail
BetsyMcPhail previously approved these changes Sep 16, 2024
Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion, I was experimenting and accidentally pushed to this branch. I was hoping to get a cleaner failure, with a report of the command, push the results to CDash, etc. I do have a WIP (#290) that should be ready soon. Should I push to this branch directly?

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @BetsyMcPhail)

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, pushing here is fine. Let me know if you need me to help test, or anything.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @BetsyMcPhail)

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @BetsyMcPhail and @rpoyner-tri)

a discussion (no related file):
Just adding a blocked discussion so it doesn't get merged before Betsy is satisfied.


Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpoyner-tri how do these test results look to you?

Test at r4 (with small change to speed up testing) and breaking change in drake (RobotLocomotion/drake#21915): https://drake-jenkins.csail.mit.edu/view/Coverage/job/linux-jammy-clang-bazel-experimental-coverage/37/

Test at r4 with drake master: https://drake-jenkins.csail.mit.edu/view/Coverage/job/linux-jammy-clang-bazel-experimental-coverage/38/

+@mwoehlke-kitware for CMake code review

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @mwoehlke-kitware)

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @BetsyMcPhail and @mwoehlke-kitware)

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.

3 participants