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

spdlog: Use system library if found #154

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

frantisekz
Copy link
Contributor

Distributions prefer to use system libraries if present, this will make level-zero use the system spdlog if one is found, falls back to the bundled one otherwise.

source/utils/CMakeLists.txt Outdated Show resolved Hide resolved
source/utils/CMakeLists.txt Outdated Show resolved Hide resolved
@lisanna-dettwyler
Copy link
Contributor

We'll accept this if you place the lookup behind a cmake flag that distros can enable if they want to. We want the bundled headers to be used by default.

@LecrisUT
Copy link

We'll accept this if you place the lookup behind a cmake flag that distros can enable if they want to. We want the bundled headers to be used by default.

It is customizable with CMAKE_DISABLE_FIND_PACKAGE_spdlog, but the default would still be the opposite. Could you elaborate on what your concerns are with having default in one way or the other? Typically I see projects implement fallthrough mechanism, or more modern is to rely on FetchContent(FIND_PACKAGE_ARGS) which gives maximum control to users, developers and packagers. (It can work offline with local/submodule third-party sources)

@lisanna-dettwyler
Copy link
Contributor

It is customizable with CMAKE_DISABLE_FIND_PACKAGE_spdlog, but the default would still be the opposite. Could you elaborate on what your concerns are with having default in one way or the other? Typically I see projects implement fallthrough mechanism, or more modern is to rely on FetchContent(FIND_PACKAGE_ARGS) which gives maximum control to users, developers and packagers. (It can work offline with local/submodule third-party sources)

If we default to system headers, at best nothing changes, but at worst a breaking change gets introduced.

@LecrisUT
Copy link

LecrisUT commented Jun 14, 2024

but at worst a breaking change gets introduced.

spdlog is responsible to make sure they don't have breaking changes in the same major version as they are using COMPATIBILITY SameMajorVersion
https://github.com/gabime/spdlog/blob/c3aed4b68373955e1cc94307683d44dca1515d2b/CMakeLists.txt#L356

Version checking is handled by simply adding a 1.x.y version for the find_package argument

@frantisekz
Copy link
Contributor Author

We'll accept this if you place the lookup behind a cmake flag that distros can enable if they want to. We want the bundled headers to be used by default.

I've added this behavior, if SYSTEM_SPDLOG flag isn't specified, the project defaults to use the bundled spdlog.

@lisanna-dettwyler
Copy link
Contributor

One last thing, please re-fetch and add signed-off-by line to the commit.

@frantisekz
Copy link
Contributor Author

One last thing, please re-fetch and add signed-off-by line to the commit.

Aaand done :)

Signed-off-by: František Zatloukal <fzatlouk@redhat.com>
@lisanna-dettwyler lisanna-dettwyler merged commit a65b332 into oneapi-src:master Jun 25, 2024
21 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants