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

Add fmt 9.1.0 #364

Merged
merged 8 commits into from
Feb 8, 2023
Merged

Add fmt 9.1.0 #364

merged 8 commits into from
Feb 8, 2023

Conversation

kkraus14
Copy link
Contributor

@kkraus14 kkraus14 commented Feb 6, 2023

Description

Adds fmt 9.1.0 to rapids-cmake via rapids_cpm_fmt based on discussion in rapidsai/rmm#1177. Nothing should be using rapids_cpm_fmt yet so we don't need to version align it with spdlog until spdlog is updated to 1.11.0.

Depends on #366

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 6, 2023 22:52
@rapids-bot
Copy link

rapids-bot bot commented Feb 6, 2023

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

GIT_SHALLOW ${shallow}
PATCH_COMMAND ${patch_command}
EXCLUDE_FROM_ALL ${exclude}
OPTIONS "FMT_INSTALL ${to_install}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to this option there's a FMT_SYSTEM_HEADERS option that treats them as system includes instead of user includes presumably to make things like clang-tidy happy. If we want that option I'm happy to add it here (as well as for spdlog which also has the option).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what to say here. @vyasr @robertmaynard?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does anyone know what the behavior of spdlog is when using an internal version of fmt? If it is marking the headers as system I don't mind re-producing that approach.

This will only effect when we are building fmt from source, since installed versions of fmt will always be treated as system due to how IMPORT targets in CMake behave.

GIT_SHALLOW ${shallow}
PATCH_COMMAND ${patch_command}
EXCLUDE_FROM_ALL ${exclude}
OPTIONS "FMT_INSTALL ${to_install}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what to say here. @vyasr @robertmaynard?

cmake-format-rapids-cmake.json Outdated Show resolved Hide resolved
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.

This looks fine to me.

@kkraus14 To revisit, you plan to open a follow-up PR to update the rapids-cmake pinning of spdlog and fetch fmt as a dependency? rapidsai/rmm#1177 (comment)

After that, wheel builds should start passing for rapidsai/rmm#1177 as I understand?

I can merge once you confirm the plan.

@bdice
Copy link
Contributor

bdice commented Feb 8, 2023

/ok to test

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

kkraus14 commented Feb 8, 2023

@kkraus14 To revisit, you plan to open a follow-up PR to update the rapids-cmake pinning of spdlog and fetch fmt as a dependency? rapidsai/rmm#1177 (comment)

Yes, that's correct. Updating that pinning needs to be coordinated with the RMM pr.

@bdice
Copy link
Contributor

bdice commented Feb 8, 2023

/merge

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