Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 logr v0.1.0 #3812
Add logr v0.1.0 #3812
Changes from 2 commits
edbe6f6
9bdaddf
4fd9803
7299203
43171e1
91739d8
8f11c5b
24d586c
a00beb0
06c8d8c
4db869d
ece574b
f1b4e53
e280a7e
6750c8f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you please rename these options to:
spdlog
,glog
andlog4cplus
.Also can all 3 backends be used together, or are they to be used exclusive?
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.
The meaning of this options is whether the corresponding header file would be installed or not. By default all of them are present.
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 surprised you don't add a requirement on either spdlog, glog or log4plus.
If this is a frontend substitution, then the backend should be available. Isn't it?
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 consider the following reasoning:
Deciding the logger happens in the first place (so deps machinery is applied for logger lib), and the logr is like an extension to a selected library.
And in case of using in conanfile it would be less bothering to just specify logr without forcing the user to dig into options.
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.
The problem is that
logr
won't work when having the following conanfile.txt:because spdlog is is not available.
I would do this in
requirements
: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've tested the following conanfile.txt
And a project based on test_project works just fine.
The libs is a collection of headers and it has some options to add a certain headers or not.
Would it be ok to just remove all options and make all headers to be unconditionally present?
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 usable is this library without any backend?
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.
That could be a way, but I think you should also add these library as a requirement of logr. Or shouldn't you?
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.
all is the option for local dev builds. As I want all of them to be available in single build (for examples and benchmarks) and I want to keep local conanfile at most similar to projects own conanfile.
Does Conan introduces dependencie propogation when I specify
self.requires("spdlog/x.y")
? Or I need to modify projects cmake files and add something liketarget_link_libraries(logr INTERFACE spdlog::spdlog)
?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.
Personally, I have given up on keeping project's conanfiles equal to cci's conanfiles.
Mostly because I don't want the project to need a cmake wrapper script, just for conan.
When you add
self.requires("spdlog/x.y")
to logr,then linking to the
logr::logr
cmake target will also link tospdlog::spdlog
.I changed your example a bit by removing the list from
default_options
.It can only have one.
As @prince-chrismc already mentioned, I don't think an
all
option does not make muck sense for cci. For your personal development branch, it might.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.
Yep! CCI is trying to provide the most convince to as many consumers as possible. Which may not meet the needs of the project!
That being said, as a very happy RESTinio consumer, it seems unlikely that a project will have multiple logging methods.... but then they wouldn't need this library either!
✅ There is no right or wrong answer, so ultimately the choice is yours.
I have to agree with @madebr suggestion for a single option that adds the correct requires... for myself this adds the freedom to change in the future with little impact.
Again the opinion of a potential consumer =)
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.
Refactored to a single option "backend", which implies dependency to a specific logger 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.
An optimisation we like to add is
no_copy_source
attribute near the options.On mobile so I do not have an example
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've tried to set no_copy_source, but then I have to use projects own root CMakeLists.txt, and it doesn't workout great as I use "cmake_find_package" generator there and some cmake-scripts are in submodule wich is not a part of tarball. For next release I'll handle those issues and will apply this options.