-
Notifications
You must be signed in to change notification settings - Fork 605
Bump gz-transport(14) to 14.2.0 #6536
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
Conversation
Signed-off-by: Shameek Ganguly <[email protected]>
|
Hello @bazelbuild/bcr-maintainers, modules (gz-transport) have been updated in this PR. |
|
@bazel-io skip_check unstable_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request bumps gz-transport to version 14.2.0. The changes include adding the new module version with its dependencies, presubmit configuration, and patches to improve Bazel support and enable tests. My review found a critical issue with the source URL not matching the declared version, which needs to be addressed. I also suggest an improvement for handling flaky tests to avoid disabling them completely.
| "url": "https://github.com/gazebosim/gz-transport/archive/refs/tags/gz-transport14_14.1.0.tar.gz", | ||
| "integrity": "sha256-OZPWJy6wxaLslIB7GK4CCRuKGxcjo0WoMzLeNne6/5o=", | ||
| "strip_prefix": "gz-transport-gz-transport14_14.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module version is 14.2.0, but the url and strip_prefix in source.json point to the source code for version 14.1.0. This is misleading and can cause confusion, as the downloaded source code should match the module version.
- If
14.2.0is an official upstream release, please update theurlandstrip_prefixto point to the correct14.2.0source archive. - If
14.2.0is not an official release but represents a state from a development branch (e.g.,gz-transport14), please use a URL pointing to a specific commit hash that includes the desired changes. You might also consider using a pseudo-version (e.g.,14.2.0-pre.1) to indicate it's not a final release. - If these changes are considered patches on top of
14.1.0, the version should be14.1.0.bcr.2to follow BCR conventions for patched versions.
| +[cc_test( | ||
| + name = "INTEGRATION_" + src.replace(".cc", ""), | ||
| + srcs = ["integration/" + src], | ||
| + data = [":" + name for name in test_executables], | ||
| + defines = [ | ||
| + 'AUTH_PUB_SUB_SUBSCRIBER_INVALID_EXE=\\"./test/authPubSubSubscriberInvalid_aux\\"', | ||
| + 'FAST_PUB_EXE=\\"./test/fastPub_aux\\"', | ||
| + 'PUB_EXE=\\"./test/pub_aux\\"', | ||
| + 'PUB_THROTTLED_EXE=\\"./test/pub_aux_throttled\\"', | ||
| + 'SCOPED_TOPIC_SUBSCRIBER_EXE=\\"./test/scopedTopicSubscriber_aux\\"', | ||
| + 'TWO_PROCS_PUBLISHER_EXE=\\"./test/twoProcsPublisher_aux\\"', | ||
| + 'TWO_PROCS_PUB_SUB_MIXED_SUBSCRIBERS_EXE=\\"./test/twoProcsPubSubMixedSubscribers_aux\\"', | ||
| + 'TWO_PROCS_PUB_SUB_SINGLE_SUBSCRIBER_EXE=\\"./test/twoProcsPubSubSingleSubscriber_aux\\"', | ||
| + 'TWO_PROCS_PUB_SUB_SUBSCRIBER_EXE=\\"./test/twoProcsPubSubSubscriber_aux\\"', | ||
| + 'TWO_PROCS_SRV_CALL_REPLIER_EXE=\\"./test/twoProcsSrvCallReplier_aux\\"', | ||
| + 'TWO_PROCS_SRV_CALL_REPLIER_INC_EXE=\\"./test/twoProcsSrvCallReplierInc_aux\\"', | ||
| + 'TWO_PROCS_SRV_CALL_WITHOUT_INPUT_REPLIER_EXE=\\"./test/twoProcsSrvCallWithoutInputReplier_aux\\"', | ||
| + 'TWO_PROCS_SRV_CALL_WITHOUT_INPUT_REPLIER_INC_EXE=\\"./test/twoProcsSrvCallWithoutInputReplierInc_aux\\"', | ||
| + 'TWO_PROCS_SRV_CALL_WITHOUT_OUTPUT_REPLIER_EXE=\\"./test/twoProcsSrvCallWithoutOutputReplier_aux\\"', | ||
| + 'TWO_PROCS_SRV_CALL_WITHOUT_OUTPUT_REPLIER_INC_EXE=\\"./test/twoProcsSrvCallWithoutOutputReplierInc_aux\\"', | ||
| + ], | ||
| + env = {"GZ_BAZEL": "1"}, | ||
| + deps = [ | ||
| + ":Config", | ||
| + ":utils", | ||
| + "//:gz-transport", | ||
| + "@googletest//:gtest", | ||
| + "@googletest//:gtest_main", | ||
| + "@gz-msgs//:gzmsgs_cc_proto", | ||
| + "@gz-utils//:Environment", | ||
| + "@gz-utils//:Subprocess", | ||
| + ], | ||
| +) for src in integration_test_sources if src not in flaky_test_srcs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of completely excluding flaky tests from being built and run by filtering them out, it's better to mark them as flaky using the flaky = True attribute in the cc_test rule. This ensures the tests are still executed, providing a signal on their status, but their failures won't fail the overall build. This practice helps in tracking their flakiness over time and encourages them to be fixed.
[cc_test(
name = "INTEGRATION_" + src.replace(".cc", ""),
srcs = ["integration/" + src],
data = [":" + name for name in test_executables],
defines = [
'AUTH_PUB_SUB_SUBSCRIBER_INVALID_EXE=\\"./test/authPubSubSubscriberInvalid_aux\\"',
'FAST_PUB_EXE=\\"./test/fastPub_aux\\"',
'PUB_EXE=\\"./test/pub_aux\\"',
'PUB_THROTTLED_EXE=\\"./test/pub_aux_throttled\\"',
'SCOPED_TOPIC_SUBSCRIBER_EXE=\\"./test/scopedTopicSubscriber_aux\\"',
'TWO_PROCS_PUBLISHER_EXE=\\"./test/twoProcsPublisher_aux\\"',
'TWO_PROCS_PUB_SUB_MIXED_SUBSCRIBERS_EXE=\\"./test/twoProcsPubSubMixedSubscribers_aux\\"',
'TWO_PROCS_PUB_SUB_SINGLE_SUBSCRIBER_EXE=\\"./test/twoProcsPubSubSingleSubscriber_aux\\"',
'TWO_PROCS_PUB_SUB_SUBSCRIBER_EXE=\\"./test/twoProcsPubSubSubscriber_aux\\"',
'TWO_PROCS_SRV_CALL_REPLIER_EXE=\\"./test/twoProcsSrvCallReplier_aux\\"',
'TWO_PROCS_SRV_CALL_REPLIER_INC_EXE=\\"./test/twoProcsSrvCallReplierInc_aux\\"',
'TWO_PROCS_SRV_CALL_WITHOUT_INPUT_REPLIER_EXE=\\"./test/twoProcsSrvCallWithoutInputReplier_aux\\"',
'TWO_PROCS_SRV_CALL_WITHOUT_INPUT_REPLIER_INC_EXE=\\"./test/twoProcsSrvCallWithoutInputReplierInc_aux\\"',
'TWO_PROCS_SRV_CALL_WITHOUT_OUTPUT_REPLIER_EXE=\\"./test/twoProcsSrvCallWithoutOutputReplier_aux\\"',
'TWO_PROCS_SRV_CALL_WITHOUT_OUTPUT_REPLIER_INC_EXE=\\"./test/twoProcsSrvCallWithoutOutputReplierInc_aux\\"',
],
env = {"GZ_BAZEL": "1"},
flaky = src in flaky_test_srcs,
deps = [
":Config",
":utils",
"//:gz-transport",
"@googletest//:gtest",
"@googletest//:gtest_main",
"@gz-msgs//:gzmsgs_cc_proto",
"@gz-utils//:Environment",
"@gz-utils//:Subprocess",
],
) for src in integration_test_sources]
bazel-io
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All modules in this PR have been approved by their maintainers. This PR will be merged if all presubmit checks pass.
All applies gazebosim/gz-transport#745 and gazebosim/gz-transport#747 as bazel_fixes.patch