From d4066fa611c803430c9bc5dbe8e243f89bb9a25c Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 27 Nov 2024 06:42:20 -0800 Subject: [PATCH] Fix some logger issues (#1739) This fixes a handful of issues uncovered in downstream CI after #1722. The following are bugs introduced in #1722: - When using rmm from the build directory rather than an installation, the namespaced targets are not present so we must generate aliases. - The `RMM_LOGGING_ASSERT` macro is never used in rmm itself, so we didn't catch that it was still using the old version of the logger. While fixing the above, I also uncovered that building fmt in this environment unearths a gcc bug. The following are underlying issues uncovered by #1722: - spdlog's fmt CMake linkage is determined at build time. As a result, the conda package for spdlog is hardcoded to use fmt as a library (static or shared depends on what the `fmt::fmt` target winds up being when a consumer using spdlog finds fmt in CMake), which means that is propagated to all consumers of the librmm package via its CMake. This means that we often wind up with both fmt_header_only and fmt as link targets for many RAPIDS libraries. For now, this PR makes it so that if `rapids_cpm_find(spdlog)` does not find a copy of spdlog locally, the cloned version will use an external header-only fmt via rapids-cmake's logic, which ensures that packages like wheels do not export a libfmt or libspdlog dependency. However, in environments where `rapids_cpm_find(spdlog)` does find a preexisting package, we allow that package's fmt linkage to propagate. In conda environments, we know that this fmt linkage is to the library, so we keep fmt as part of rmm's runtime dependencies (by placing it in host and relying on the run export) so that libfmt is always available in environments using rmm. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) URL: https://github.com/rapidsai/rmm/pull/1739 --- CMakeLists.txt | 2 ++ ci/check_symbols.sh | 4 +++- cmake/thirdparty/get_spdlog.cmake | 8 ++++++++ conda/environments/all_cuda-118_arch-x86_64.yaml | 1 - conda/environments/all_cuda-125_arch-x86_64.yaml | 1 - conda/recipes/librmm/meta.yaml | 14 +++++++------- dependencies.yaml | 1 - include/rmm/detail/logging_assert.hpp | 2 +- rapids_logger/logger_impl.hpp.in | 6 ++++++ 9 files changed, 27 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7a7ba3525..8c5bd5428 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,6 +75,8 @@ rapids_cpm_init() add_subdirectory(rapids_logger) rapids_make_logger(rmm EXPORT_SET rmm-exports) +add_library(rmm::rmm_logger ALIAS rmm_logger) +add_library(rmm::rmm_logger_impl ALIAS rmm_logger_impl) include(cmake/thirdparty/get_cccl.cmake) include(cmake/thirdparty/get_nvtx.cmake) diff --git a/ci/check_symbols.sh b/ci/check_symbols.sh index 155e509da..377a93cac 100755 --- a/ci/check_symbols.sh +++ b/ci/check_symbols.sh @@ -53,7 +53,9 @@ for dso_file in ${dso_files}; do fi echo "checking for 'spdlog::' symbols..." - if grep -E 'spdlog\:\:' < "${symbol_file}"; then + if grep -E 'spdlog\:\:' < "${symbol_file}" \ + | grep -v 'std\:\:_Destroy_aux' + then raise-symbols-found-error 'spdlog::' fi echo "No symbol visibility issues found" diff --git a/cmake/thirdparty/get_spdlog.cmake b/cmake/thirdparty/get_spdlog.cmake index 212f604c3..febdf4c5c 100644 --- a/cmake/thirdparty/get_spdlog.cmake +++ b/cmake/thirdparty/get_spdlog.cmake @@ -17,6 +17,14 @@ function(find_and_configure_spdlog) include(${rapids-cmake-dir}/cpm/spdlog.cmake) rapids_cpm_spdlog( + # The conda package for fmt is hard-coded to assume that we use a preexisting fmt library. This + # is why we have always had a libfmt linkage despite choosing to specify the header-only version + # of fmt. We need a more robust way of modifying this to support fully self-contained build and + # usage even in environments where fmt and/or spdlog are already present. The crudest solution + # would be to modify the interface compile definitions and link libraries of the spdlog target, + # if necessary. For now I'm specifying EXTERNAL_FMT_HO here so that in environments where spdlog + # is cloned and built from source we wind up with the behavior that we expect, but we'll have to + # resolve this properly eventually. FMT_OPTION "EXTERNAL_FMT_HO" INSTALL_EXPORT_SET rmm-exports BUILD_EXPORT_SET rmm-exports) diff --git a/conda/environments/all_cuda-118_arch-x86_64.yaml b/conda/environments/all_cuda-118_arch-x86_64.yaml index 519c056b5..ad2cbf9e6 100644 --- a/conda/environments/all_cuda-118_arch-x86_64.yaml +++ b/conda/environments/all_cuda-118_arch-x86_64.yaml @@ -35,7 +35,6 @@ dependencies: - python>=3.10,<3.13 - rapids-build-backend>=0.3.0,<0.4.0.dev0 - scikit-build-core >=0.10.0 -- spdlog>=1.14.1,<1.15 - sphinx - sphinx-copybutton - sphinx-markdown-tables diff --git a/conda/environments/all_cuda-125_arch-x86_64.yaml b/conda/environments/all_cuda-125_arch-x86_64.yaml index 86e887c21..520c7d743 100644 --- a/conda/environments/all_cuda-125_arch-x86_64.yaml +++ b/conda/environments/all_cuda-125_arch-x86_64.yaml @@ -34,7 +34,6 @@ dependencies: - python>=3.10,<3.13 - rapids-build-backend>=0.3.0,<0.4.0.dev0 - scikit-build-core >=0.10.0 -- spdlog>=1.14.1,<1.15 - sphinx - sphinx-copybutton - sphinx-markdown-tables diff --git a/conda/recipes/librmm/meta.yaml b/conda/recipes/librmm/meta.yaml index 53e16ebdc..31aaf0e63 100644 --- a/conda/recipes/librmm/meta.yaml +++ b/conda/recipes/librmm/meta.yaml @@ -26,13 +26,13 @@ requirements: - {{ stdlib("c") }} host: - cuda-version ={{ cuda_version }} - # We require spdlog and fmt (which was de-vendored from spdlog - # conda-forge packages in 1.11.0) so that the spdlog headers are not - # pulled by CPM and installed as a part of the rmm packages. However, - # building against librmm still requires these headers. They are also - # added as a run requirement via the packages' run_exports. + # We need fmt here for now because the conda spdlog package is hard-coded + # to use fmt as a compiled library, not header-only, so we must ensure that + # the library is present for now so that if a downstream library tries to + # build against rmm and some other package in its build environment uses + # fmt (or spdlog) that the default rmm build is consistent with such + # environments. - fmt {{ fmt_version }} - - spdlog {{ spdlog_version }} build: script_env: @@ -77,8 +77,8 @@ outputs: {% if cuda_major == "11" %} - cudatoolkit {% endif %} + # See comment about fmt in the build section above. - fmt {{ fmt_version }} - - spdlog {{ spdlog_version }} test: commands: - test -d "${PREFIX}/include/rmm" diff --git a/dependencies.yaml b/dependencies.yaml index 070248edb..f92268639 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -112,7 +112,6 @@ dependencies: - c-compiler - cxx-compiler - fmt>=11.0.2,<12 - - spdlog>=1.14.1,<1.15 specific: - output_types: conda matrices: diff --git a/include/rmm/detail/logging_assert.hpp b/include/rmm/detail/logging_assert.hpp index c3b12ffe3..d5b2ca10a 100644 --- a/include/rmm/detail/logging_assert.hpp +++ b/include/rmm/detail/logging_assert.hpp @@ -38,7 +38,7 @@ if (!success) { \ RMM_LOG_CRITICAL( \ "[" __FILE__ ":" RMM_STRINGIFY(__LINE__) "] Assertion " RMM_STRINGIFY(_expr) " failed."); \ - rmm::detail::logger().flush(); \ + rmm::default_logger().flush(); \ /* NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) */ \ assert(success); \ } \ diff --git a/rapids_logger/logger_impl.hpp.in b/rapids_logger/logger_impl.hpp.in index 717a00ac9..d5b467571 100644 --- a/rapids_logger/logger_impl.hpp.in +++ b/rapids_logger/logger_impl.hpp.in @@ -26,10 +26,16 @@ // Start hiding before including spdlog headers. #pragma GCC visibility push(hidden) +// This issue claims to have been resolved in gcc 8, but we still seem to encounter it here. +// The code compiles and links and all tests pass, and nm shows symbols resolved as expected. +// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80947 +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wattributes" #include #include #include +#pragma GCC diagnostic pop #include #include