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

Bump spdlog to 1.11, add fmt as dependency for spdlog #368

Merged
merged 8 commits into from
Feb 13, 2023

Conversation

kkraus14
Copy link
Contributor

@kkraus14 kkraus14 commented Feb 9, 2023

Description

Bumps spdlog to 1.11.0 and adds fmt as a dependency of spdlog and sets the SPDLOG_FMT_EXTERNAL_HO cmake option for spdlog. Happy to change that to be controlled by an argument to the rapids_cpm_spdlog function if desired. I believe the existing testing should cover the changes made here as the spdlog and rmm related tests were failing until adding fmt to the spdlog BUILD_EXPORT_SET and INSTALL_EXPORT_SET.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@kkraus14 kkraus14 requested a review from a team as a code owner February 9, 2023 00:17
@rapids-bot
Copy link

rapids-bot bot commented Feb 9, 2023

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

@bdice bdice added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Feb 9, 2023
@bdice
Copy link
Contributor

bdice commented Feb 9, 2023

/ok to test

bdice
bdice previously requested changes Feb 10, 2023
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for now with two small comments. I am going to wait to merge this until I can babysit the RMM PR through to completion, to minimize potential downtime / downstream breakage. Probably next Monday.

rapids-cmake/cpm/spdlog.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/spdlog.cmake Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Feb 10, 2023

/ok to test

@kkraus14
Copy link
Contributor Author

This doesn't seem to be working properly in the case where spdlog and fmt aren't installed already locally when trying to build RMM:

CMake Error in CMakeLists.txt:
  export called with target "rmm" which requires target "fmt-header-only"
  that is not in this export set, but in multiple other export sets:
  /home/keith/git/rmm/build/_deps/fmt-build/fmt-targets.cmake,
  /home/keith/git/rmm/build/fmt-targets.cmake.

  An exported target cannot depend upon another target which is exported
  multiple times.  Consider consolidating the exports of the
  "fmt-header-only" target to a single export.

I think this is because we're depending on both spdlog and fmt and we transitively export fmt when exporting spdlog which causes fmt to be double exported? I would assume the solution to this is to pass on the export arguments into the rapids_cpm_fmt call in the rapids_cpm_spdlog call conditionally, but I'm not quite sure. @robertmaynard maybe you have an idea?

@robertmaynard
Copy link
Contributor

I think this is because we're depending on both spdlog and fmt and we transitively export fmt when exporting spdlog which causes fmt to be double exported? I would assume the solution to this is to pass on the export arguments into the rapids_cpm_fmt call in the rapids_cpm_spdlog call conditionally, but I'm not quite sure. @robertmaynard maybe you have an idea?

We are doing multiple exports of the fmt package since it is also doing a call to export(. The correct approach it to remove our export in get_fmt ( https://github.com/rapidsai/rmm/pull/1177/files#r1104528086 ).

@robertmaynard
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 9f27732 into rapidsai:branch-23.04 Feb 13, 2023
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Feb 13, 2023
PR #1177 was merged a little too early when CI passed due to the presence of a `/merge` comment and sufficient approvals. This reverts a temporary change to the rapids-cmake repo that is no longer needed because rapidsai/rapids-cmake#368 has been merged.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)

URL: #1209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants