Skip to content

Allow nvbench to use an exiting fmt when brought in via find_package#125

Closed
robertmaynard wants to merge 1 commit intoNVIDIA:mainfrom
robertmaynard:use_existing_fmt
Closed

Allow nvbench to use an exiting fmt when brought in via find_package#125
robertmaynard wants to merge 1 commit intoNVIDIA:mainfrom
robertmaynard:use_existing_fmt

Conversation

@robertmaynard
Copy link
Collaborator

No description provided.

@robertmaynard robertmaynard added type: bug: functional Does not work as intended. only: cmake CMake changes only. helps: rapids Helps or needed by RAPIDS. labels Feb 14, 2023
@@ -1,6 +1,7 @@
################################################################################
# fmtlib/fmt
rapids_cpm_find(fmt 9.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nvbench originally requested that feature in rapidsai/rapids-cmake#101

Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

LGTM, @robertmaynard feel free to merge after addressing @bdice's question.

@robertmaynard
Copy link
Collaborator Author

LGTM, @robertmaynard feel free to merge after addressing @bdice's question.

Are you okay moving to rapids-cmake 23.06 to get this feature? It would mean moving your minimum CMake version to 3.23.1 compared to the existing 3.20

@bdice
Copy link
Contributor

bdice commented Jul 20, 2023

@robertmaynard Can we finalize this PR?

@robertmaynard
Copy link
Collaborator Author

@robertmaynard Can we finalize this PR?

There is still open questions around the update to using rapids_cpm_fmt requiring nvbench to move to a newer rapids-cmake version, which would bump the CMake minimum version. If we are all fine with dropping that request ( or doing the bump ) I am happy to merge this.

@bdice
Copy link
Contributor

bdice commented Jul 20, 2023

I'd support making this update by bumping the CMake minimum. @allisonvacanti Do you have any reservations about that?

If there are issues with that, let's merge as-is without using rapids_cpm_fmt. This change is in a patch downstream (in cudf) that would be nice to drop, one way or another.

@robertmaynard
Copy link
Collaborator Author

Closing as these changes are now part of #140

@robertmaynard robertmaynard deleted the use_existing_fmt branch October 17, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

helps: rapids Helps or needed by RAPIDS. only: cmake CMake changes only. type: bug: functional Does not work as intended.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants