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

[BUG] Cannot be built in an environment where fmt and spdlog are installed independently. #904

Closed
gdaisukesuzuki opened this issue Nov 4, 2021 · 6 comments · Fixed by #907
Labels
? - Needs Triage Need team to review and classify bug Something isn't working

Comments

@gdaisukesuzuki
Copy link

File bug happens
arena.hpp
arena_memory_resource.hpp

Issues to be fixed
Both header files are assumed to be built on spdlog, where fmt is bundled, and not in an environment where fmt and spglog are installed separately.

Suggested patch

*** rmm/include/rmm/mr/device/detail/arena.hpp-dist     2021-11-03 23:02:58.084936643 +0900
--- rmm/include/rmm/mr/device/detail/arena.hpp  2021-11-03 23:04:31.726165374 +0900
***************
*** 24,30 ****
  #include <cuda_runtime_api.h>
  
  #include <spdlog/common.h>
! #include <spdlog/fmt/bundled/ostream.h>
  
  #include <algorithm>
  #include <cstddef>
--- 24,35 ----
  #include <cuda_runtime_api.h>
  
  #include <spdlog/common.h>
! #if !defined(SPDLOG_FMT_EXTERNAL)
! #      include <spdlog/fmt/bundled/ostream.h>
! #else
! #      include <fmt/ostream.h>
! #endif
! 
  
  #include <algorithm>
  #include <cstddef>
*** rmm/include/rmm/mr/device/arena_memory_resource.hpp-dist    2021-11-03 23:53:12.040547640 +0900
--- rmm/include/rmm/mr/device/arena_memory_resource.hpp 2021-11-03 23:54:49.082345112 +0900
***************
*** 23,29 ****
  #include <cuda_runtime_api.h>
  
  #include <spdlog/common.h>
! #include <spdlog/fmt/bundled/ostream.h>
  
  #include <cstddef>
  #include <map>
--- 23,34 ----
  #include <cuda_runtime_api.h>
  
  #include <spdlog/common.h>
! #if !defined(SPDLOG_FMT_EXTERNAL)
! #      include <spdlog/fmt/bundled/ostream.h>
! #else
! #      include <fmt/ostream.h>
! #endif
! 
  
  #include <cstddef>
  #include <map>

@gdaisukesuzuki gdaisukesuzuki added ? - Needs Triage Need team to review and classify bug Something isn't working labels Nov 4, 2021
@harrism
Copy link
Member

harrism commented Nov 4, 2021

I think it would be nice to just be able to use the #include <fmt/ostream.h> always. So we should either depend on fmt directly, or add a -I compiler option pointing to the location of spdlog.

@harrism
Copy link
Member

harrism commented Nov 4, 2021

@robertmaynard given we get spdlog with cpm, is there a simple way in cmake to add -I<spdlog_install_dir>/fmt to the command line?

@harrism
Copy link
Member

harrism commented Nov 4, 2021

Just realized spdlog puts its bundled fmt in spdlog/fmt/bundled, not just in spdlog/fmt. So maybe that won't work.

@gdaisukesuzuki
Copy link
Author

I think the way the 2 files should be modified as the following file (which calls fmt) is written.

/usr/include/spdlog/fmt/fmt.h

@robertmaynard
Copy link
Contributor

It looks like spdlog provides wrapper headers that should be used instead of directly including anything in the bundled directory.

For example for ostream we should be using spdlog/fmt/osr.h which computes the correct header based on SPDLOG_FMT_EXTERNAL

@robertmaynard
Copy link
Contributor

#907 Should resolve this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
? - Needs Triage Need team to review and classify bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants