Copilot commented on code in PR #3313:
URL: https://github.com/apache/brpc/pull/3313#discussion_r3318821606
##########
registry/modules/libunwind/1.8.3.brpc-no-unwind/source.json:
##########
@@ -3,7 +3,7 @@
"strip_prefix": "libunwind-1.8.3",
"overlay": {
"BUILD.bazel": "sha256-ozAABv0I3+tacve4z2m+lAh/lhJWh7t7L3unh8FSfp8=",
- "MODULE.bazel": "sha256-EtBEsSDMJgdmCs7ByJxVP2SSVc+E7kUVLgpI3TEh/9M="
+ "MODULE.bazel": "qimDkl8soxNM/JuqndzGOOcn5ZwGvo2J6CSsNS2/IQs="
Review Comment:
The `MODULE.bazel` integrity value is missing the `sha256-` prefix that the
sibling `BUILD.bazel` entry (and the top-level `integrity` field) use. Bazel's
module resolver requires the `<algo>-` prefix on Subresource-Integrity strings;
without it, fetching this overlay file will fail integrity verification. Add
the `sha256-` prefix so the value reads
`sha256-qimDkl8soxNM/JuqndzGOOcn5ZwGvo2J6CSsNS2/IQs=`.
##########
MODULE.bazel:
##########
@@ -16,8 +16,17 @@ bazel_dep(name = "apple_support", version = "1.17.1")
bazel_dep(name = 'rules_cc', version = '0.0.1')
bazel_dep(name = 'rules_proto', version = '4.0.0')
bazel_dep(name = 'zlib', version = '1.3.1.bcr.5', repo_name =
'com_github_madler_zlib')
-bazel_dep(name = 'libunwind', version = '1.8.1', repo_name =
'com_github_libunwind_libunwind')
bazel_dep(name = 'babylon', version = '1.4.4')
+# --registry=https://raw.githubusercontent.com/apache/brpc/master/registry
+# `registry/modules/libunwind/` (see the
`--registry=https://raw.githubusercontent.com/apache/brpc/master/registry`
+# entry at the top of `.bazelrc`). The version suffix `.brpc-no-unwind` marks
+# this as brpc's self-maintained variant whose `src/unwind/*.c` (the GCC
+# `_Unwind_*` ABI compatibility layer) is gated by the
+# `--define=libunwind_hide_unwind_symbols=true` switch. Hiding those
+# `_Unwind_*` symbols is required for `--define=with_bthread_tracer=true`
+# to work without crashing in pthread_exit / exception unwinding.
+# See docs/cn/bthread_tracer.md for background.
+bazel_dep(name = 'libunwind', version = '1.8.1.brpc-no-unwind', repo_name =
'com_github_libunwind_libunwind')
Review Comment:
The `MODULE.bazel` declares `version = '1.8.1.brpc-no-unwind'`, but the
surrounding comment (and the new overlay added in this PR under
`registry/modules/libunwind/1.8.3.brpc-no-unwind/`) refers to
`1.8.3.brpc-no-unwind`. The `1.8.1.brpc-no-unwind` overlay still exists, so the
build will still resolve, but the documentation comment is inconsistent with
the actual selected version — either bump `version` to `1.8.3.brpc-no-unwind`
or update the comment to say `1.8.1.brpc-no-unwind`.
##########
.bazelrc:
##########
@@ -33,12 +50,38 @@ build --features=per_object_debug_info
# We already have absl in the build, define absl=1 to tell googletest to use
absl for backtrace.
build --define absl=1
-# For brpc.
-test --define=BRPC_BUILD_FOR_UNITTEST=true
+# For UT.
+build:test --define BRPC_BUILD_FOR_UNITTEST=true
+# Hide libunwind's `_Unwind_*` symbols so they don't preempt libgcc_s at
+# runtime. Without this, pthread_exit / C++ exception unwinding crashes
+# when libunwind.so appears earlier in the dynamic link chain.
+# See registry/modules/1.8.3/overlay/BUILD.bazel for details.
Review Comment:
The comment refers to `registry/modules/1.8.3/overlay/BUILD.bazel`, but the
actual path in this repo is
`registry/modules/libunwind/1.8.3.brpc-no-unwind/overlay/BUILD.bazel` (or
`1.8.1.brpc-no-unwind` for the currently-selected version). Please fix the path
so future maintainers can find the referenced file.
##########
test/BUILD.bazel:
##########
@@ -148,26 +136,13 @@ TEST_BUTIL_SOURCES = [
],
})
-proto_library(
- name = "test_proto",
- srcs = glob(
- [
- "*.proto",
- ],
- ),
- strip_import_prefix = "/test",
- visibility = ["//visibility:public"],
- deps = [
- "//:brpc_idl_options_proto",
- ]
-)
-
-cc_proto_library(
+# 用 brpc_proto_library 直接产出 cc_library。include 默认为当前 package（test）自身，
+# 等价于原来的 strip_import_prefix = "/test"，让 .proto 以 `import "foo.proto"` 形式引用。
Review Comment:
This comment is in Chinese while every other comment in this BUILD file (and
elsewhere in the repo's Bazel files) is in English. For codebase consistency,
please translate it to English.
##########
test/BUILD.bazel:
##########
@@ -196,104 +185,73 @@ cc_test(
":cc_test_proto",
":sstream_workaround",
":gperftools_helper",
- "//:brpc",
+ "//:butil",
+ "//:bthread",
"@com_google_googletest//:gtest",
],
)
-
cc_test(
- name = "bvar_test",
- srcs = glob(
- [
- "bvar_*_unittest.cpp",
- ],
- exclude = [
- "bvar_lock_timer_unittest.cpp",
- "bvar_recorder_unittest.cpp",
- ],
- ),
- copts = COPTS,
+ name = "bvar_unittests",
+ srcs = glob(["bvar_*_unittest.cpp"]),
deps = [
":sstream_workaround",
":gperftools_helper",
"//:bvar",
"@com_google_googletest//:gtest",
- ],
-)
-
-cc_test(
- name = "bthread_test",
- srcs = glob(
- [
- "bthread_*_unittest.cpp",
- ],
- exclude = [
- "bthread_cond_unittest.cpp",
- "bthread_execution_queue_unittest.cpp",
- "bthread_dispatcher_unittest.cpp",
- "bthread_fd_unittest.cpp",
- "bthread_mutex_unittest.cpp",
- "bthread_setconcurrency_unittest.cpp",
- # glog CHECK die with a fatal error
- "bthread_key_unittest.cpp",
- "bthread_butex_multi_tag_unittest.cpp",
- "bthread_rwlock_unittest.cpp",
- "bthread_semaphore_unittest.cpp",
- ],
- ),
- copts = COPTS,
- deps = [
- ":sstream_workaround",
- ":gperftools_helper",
- "//:brpc",
- "@com_google_googletest//:gtest",
"@com_google_googletest//:gtest_main",
- ],
+ ] + TCMALLOC_DEP_UNLESS_ASAN,
+ copts = COPTS
)
-cc_test(
- name = "brpc_prometheus_test",
- srcs = glob(
- [
- "brpc_prometheus_*_unittest.cpp",
- ],
- ),
- copts = COPTS,
+generate_unittests(
+ name = "bthread_unittests",
+ srcs = glob([
+ "bthread*_unittest.cpp",
+ ]),
deps = [
- ":cc_test_proto",
":sstream_workaround",
- "//:brpc",
+ ":gperftools_helper",
+ "//:bthread",
"@com_google_googletest//:gtest",
"@com_google_googletest//:gtest_main",
- ],
+ ] + TCMALLOC_DEP_UNLESS_ASAN,
+ copts = COPTS
)
Review Comment:
Two `cc_test` definitions in this file (lines 195-205 for `bvar_unittests`
and lines 207-220 for `bthread_unittests`/`generate_unittests`) end with `copts
= COPTS` and no trailing comma. Other rule calls in the same file (e.g. the
`cc_test` for `butil_unittests` and the call at line 249) use trailing commas
after the last argument. For consistency and to avoid diff noise on future
edits, add the trailing comma.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
