Repository navigation
Conversation
Seven headers used by the gtest sources were missing from noinst_HEADERS, so the tarball from "make dist" could not build gtest with --with-cuda --enable-gtest: uct/test_device.cc:19:10: fatal error: cuda/test_kernels.h: No such file or directory Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
| @@ -365,6 +365,7 @@ gtest_LDADD += \ | |||
| endif | |||
|
|
|||
| noinst_HEADERS = \ | |||
There was a problem hiding this comment.
minor: can we also extend test/gtest/AGENTS.md ("New tests must be listed in the relevant Makefile.am source list") to mention noinst_HEADERS? 7 headers drifted out of the list, so a one-line rule there would stop it happening again.
Rationale: REVIEW.md asks reviewers to check whether changed folders need AGENTS.md guidance for recurring patterns, and the existing bullet in test/gtest/AGENTS.md covers only source lists, which is exactly the gap this PR cleans up.
There was a problem hiding this comment.
Done in df008be: the bullet in test/gtest/AGENTS.md now also says new headers go in noinst_HEADERS so make dist ships them.
|
Residual coverage gap (worth noting, not a blocker): both |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
| @@ -365,6 +365,7 @@ gtest_LDADD += \ | |||
| endif | |||
|
|
|||
| noinst_HEADERS = \ | |||
There was a problem hiding this comment.
can we also fix the same gap in test/mpi/Makefile.am? test_cuda_check_def.h, test_cuda_common.h, test_lib_check_def.h, test_mpi_tags_def.h, test_ucp.h and test_ucx_check_def.h are not in noinst_HEADERS, so a tarball build with MPI and CUDA fails the same way; ompi.supp (used by run_mpi.sh) is also missing from EXTRA_DIST. Fine to do it in a separate PR if you prefer to keep this one gtest-only.
noinst_HEADERS = \
test_cuda_check_def.h \
test_cuda_common.h \
test_lib_check_def.h \
test_mpi_tags_def.h \
test_ucp.h \
test_ucx_check_def.h
EXTRA_DIST = ompi.supp
|
Did you or your organisation sign CLA? |
What?
Add seven headers that the gtest sources include to
noinst_HEADERSintest/gtest/Makefile.am, somake distships them:common/cuda.huct/cuda/cuda_vmm_mem_buffer.h,uct/cuda/test_kernels.h,uct/cuda/test_kernels_uct.huct/rocm/test_kernels_uct.hucp/cuda/test_kernels.h,ucp/rndv_frag_mpools.h(the last one added in UCP/RNDV: Throttle rndv fragment requests #11062)Why?
The tarball from
make diston current master can't build gtest with CUDA:The
distcheckjobs inbuildlib/tools/builds.shconfigure without CUDA or gtest, so CI doesn't catch this.How?
Tested on master
f3bca4b:make dist, unpack,contrib/configure-release --with-cuda=... --enable-gtest && make→ the error above;test/gtest/gtest, including the CUDA device tests (uct/test_device.cc).🤖 Generated with Claude Code