-
Notifications
You must be signed in to change notification settings - Fork 421
Remove spdlog from libmamba, provide libmamba-spdlog library
#4082
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
base: main
Are you sure you want to change the base?
Conversation
ca64ba1 to
c83e586
Compare
c83e586 to
2a12e45
Compare
…(used by libmambapy anb mamba/micromamba)
c2bc269 to
cbd6ef2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that one could create a PR on mamba-feedstock with the patch to anticipate packaging changes.
I agree, I'll take a look there. I will probably need your help with details @jjerphan 🙏🏽 |
|
We can iterate on conda-forge/mamba-feedstock#368. |
| get_target_property(type ${target} TYPE) | ||
| if(NOT ${type} STREQUAL "INTERFACE_LIBRARY") | ||
| target_compile_options("${target}" PRIVATE ${warnings}) | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the issue with adding warnings to an Interface library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRIVATE cannot be used with target_compile_options when the target is INTERFACE_LIBRARY (header-only). (that triggers a cmake error, to be clear)
I fixed it by not adding the warnings because usually warnings flags should be set for the target having the translation units to compile these warnings with, that's why PRIVATE is used here to not pollute the flags for the user of the library. But because we have a header-only library, we don't have translation units. We never want to pollute the user build with flags added to headers they will include. In the case of header-only libraries, warnings should be set only on the test executables.
If we wanted to, we could change the PRIVATE to INTERFACE if the target is an interface library, but then we impose our warnings to all the code using these headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I agree we should not impose our warnings to the user, so let's keep it as is.
| @PACKAGE_INIT@ | ||
|
|
||
| include("${CMAKE_CURRENT_LIST_DIR}/@[email protected]") | ||
| check_required_components("@PROJECT_NAME@") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should also contain calls to find_dependency for libmamba and spdlog
jjerphan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just minor nipicks.
this does not provide the necessary work to package libmamba-spdlog (next pr?);
What do you think of adding changes in this PR to be able to package libmamba-spdlog to test everything with conda-forge/mamba-feedstock#368?
| option(BUILD_SHARED "Build shared libmamba library" OFF) | ||
| option(BUILD_STATIC "Build static libmamba library with static linkage to its dependencies" OFF) | ||
| option(BUILD_LIBMAMBA "Build libmamba library" OFF) | ||
| option(BUILD_LIBMAMBA_SPDLOG_TESTS "Build libmamba-spdlog library tests" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we would need this to be able to distribute libmamba-spdlog independently.
| option(BUILD_LIBMAMBA_SPDLOG_TESTS "Build libmamba-spdlog library tests" OFF) | |
| option(BUILD_LIBMAMBA_SPDLOG "Build libmamba-spdlog library" OFF) | |
| option(BUILD_LIBMAMBA_SPDLOG_TESTS "Build libmamba-spdlog library tests" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially had that but realized it made no sense in the context of mamba and libmambapy and also that library is header-only so it doesnt make sense to "build" it. But we can use it as en enable/disable thing, sure, in particular if it helps with packaging.
| #ifndef MAMBA_LOGGING_SPDLOG_HPP | ||
| #define MAMBA_LOGGING_SPDLOG_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using?
| #ifndef MAMBA_LOGGING_SPDLOG_HPP | |
| #define MAMBA_LOGGING_SPDLOG_HPP | |
| #pragma once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it, although note that it is not the policy for libmamba/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do it in a separate commit to remove the endif too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the codebase homogeneous. If we decide to replace the inclusion guards with pragma once statements, that should be done for the whole codebase in a dedicated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll replace the pragma once in the impl header and we can change everything in that future pr
We had a short discussion with @JohanMabille about that when I started, as stated in the pr description the goal was not to prepare for packaging. |
jjerphan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, as you prefer; I think that we will need to have the packaging change in also if we have to make a release (are those significant?).
LGTM once the threads with Johan are resolved.
jjerphan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last suggestion.
Description
spdlogfromlibmambadependencies;libmamba-spdlogwhich provides the spdlog-basedLogHandlerfor mamba;libmambapyandmamba/micromambanow depends onlibmamba-spdlogto keep usingspdlogas a log handler;For simplicity,
libmamba-spdlogis a header-only library. This avoids many possible variations oflibmambausage to get in the way of easy packaging. As we always usespdlogas a header-only library and we just wrapps it, it shouldnt impact usage much. Also only end-user targets (usually executables) will need to decide whichLogHandlerto use so this header will probably only be compiled once.Notes:
libmamba-spdlog(next pr?);Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.