Add logr v0.1.0 #3812

ngrodzitski · 2020-12-06T19:37:52Z

Specify library name and version: logr/0.1.0

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

CLAassistant · 2020-12-06T19:37:56Z

All committers have signed the CLA.

conan-center-bot · 2020-12-06T19:42:30Z

All green in build 1 (9bdaddfddfb2c11e4d24fc0201960e18917e43c0)! 😊

logr/0.1.0: Generated 1 packages. All logs here

recipes/logr/0.1.0/conanfile.py

madebr · 2020-12-06T19:48:14Z

recipes/logr/0.1.0/conanfile.py

+    options = { 'spdlog_backend' : [True, False],
+                'glog_backend' : [True, False],
+                'log4cplus_backend' : [True, False] }
+
+    default_options = { 'spdlog_backend': True,
+                        'glog_backend': True,
+                        'log4cplus_backend': True }
+


Can you please rename these options to: spdlog, glog and log4cplus.
Also can all 3 backends be used together, or are they to be used exclusive?

The meaning of this options is whether the corresponding header file would be installed or not. By default all of them are present.

recipes/logr/0.1.0/conanfile.py

recipes/logr/0.1.0/test_package/conanfile.py

madebr · 2020-12-06T19:53:35Z

recipes/logr/0.1.0/conanfile.py

+    def _build_subfolder(self):
+        return "build_subfolder"
+
+    def requirements(self):


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?

I consider the following reasoning:

Deciding what logger to use in your application (among the three supported).

Once logger library is decided, the application adds it to the list of dependencies (so logger is already in deps list).

Then it is decided to use another "frontend" to it.

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.

The problem is that logr won't work when having the following conanfile.txt:

[requires] spdlog/0.1.0 [options] logr:with_spdlog_backend=True logr:with_glog_backend=False logr:with_log4cplus_backend=False

because spdlog is is not available.
I would do this in requirements:

if self.options.with_spdlog_backend: self.requires("spdlog/x.y") elif self.options.with_glog_backend self.requires("log4cplus/x.y") elif self.options.with_log4cplus_backend: self.requires("log4cplus/x.y")

I've tested the following conanfile.txt

[requires] logr/0.1.0 [options] logr:spdlog_backend=True logr:glog_backend=False logr:log4cplus_backend=False [generators] cmake

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?

How usable is this library without any backend?

That could be a way, but I think you should also add these library as a requirement of logr. Or shouldn't you?

options = { "backend": ["spdlog", "glog", "log4cplus", "all", None } default_options = { "backend": ["spdlog"]}

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 like target_link_libraries(logr INTERFACE spdlog::spdlog)?

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 to spdlog::spdlog.

I changed your example a bit by removing the list from default_options.
It can only have one.

options = { "backend": ["spdlog", "glog", "log4cplus", "all", None } default_options = { "backend": "spdlog"}

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.

Personally, I have given up on keeping project's conanfiles equal to cci's conanfiles.

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 =)

Refactored to a single option "backend", which implies dependency to a specific logger library.

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

conan-center-bot · 2020-12-06T20:01:10Z

All green in build 2 (4fd98031e66a7754bbbfe6398f0d549a71010b21)! 😊

logr/0.1.0: Generated 1 packages. All logs here

conan-center-bot · 2020-12-06T20:02:19Z

All green in build 3 (72992035a6c91c3990fdd8d26ff028235c7cf39d)! 😊

logr/0.1.0: Generated 1 packages. All logs here

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

conan-center-bot · 2020-12-06T20:10:18Z

An unexpected error happened and has been reported. Help is on its way! 🏇

prince-chrismc · 2020-12-06T20:11:15Z

recipes/logr/0.1.0/conanfile.py

+    default_options = { "spdlog_backend": True,
+                        "glog_backend": True,
+                        "log4cplus_backend": True }


seem's a little odd to have 3 logging method's 🤔

The relevant comment to similar comment
#3812 (comment)

recipes/logr/0.1.0/test_package/CMakeLists.txt

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

conan-center-bot · 2020-12-06T20:24:21Z

All green in build 7 (24d586c5b85a6946f21eee235d5f2c01c8521d85)! 😊

logr/0.1.0: Generated 1 packages. All logs here

recipes/logr/0.1.0/conanfile.py

prince-chrismc · 2020-12-07T03:25:03Z

recipes/logr/0.1.0/conanfile.py

+        tools.rmdir(os.path.join(self.package_folder, "lib"))
+
+    def package_id(self):
+        self.info.header_only()


An optimisation we like to add is no_copy_source attribute near the options.

On mobile so I do not have an example

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.

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

conan-center-bot · 2020-12-07T11:22:31Z

All green in build 8 (a00beb0556ff7e930df82f38fc8d9443a419c094)! 😊

logr/0.1.0: Generated 1 packages. All logs here

prince-chrismc · 2020-12-07T15:09:49Z

ericLemanissier pointed out "this PR is missing config.yml" I do not see it either

ericLemanissier · 2020-12-07T15:13:26Z

Yes, I removed my message, because I realized that config.yml is not mandatory if the folder name is identical to the version

prince-chrismc · 2020-12-07T15:25:09Z

Yes, I removed my message, because I realized that config.yml is not mandatory if the folder name is identical to the version

I was unaware that was a feature!

conan-center-bot · 2020-12-07T16:00:39Z

All green in build 9 (06c8d8cc6c0fe93c1241851a8fc578403f9dfe4f)! 😊

logr/0.1.0: Generated 1 packages. All logs here

conan-center-bot · 2020-12-07T16:05:39Z

All green in build 11 (ece574b701cc61012c7f0f4d050edc538027c372)! 😊

logr/0.1.0: Generated 1 packages. All logs here

prince-chrismc · 2020-12-08T14:22:17Z

recipes/logr/0.1.0/conanfile.py

+        elif self.options.backend == "log4cplus":
+            self.requires("log4cplus/2.0.5")
+            self.build_requires_options["log4cplus"].unicode = False
+        elif self.options.backend == "log4cplus-unicode":
+            self.requires("log4cplus/2.0.5")
+            self.build_requires_options["log4cplus"].unicode = True


This does not seem to change anything for logr https://github.com/conan-io/conan-center-index/pull/3812/files#diff-ad137baee4c6c3f95ccebfa8aa275fa0c0d5fff4e1f37e8b7ca65d2541479aeaR73

would it be possible to just have one and let the consumer set his own options?

prince-chrismc

LGTM

uilianries

Avoid forcing dependencies options, it creates an implicit package id, doesn't matter what you pass, it forces another package id. Instead, raise an error to warn the user.

recipes/logr/0.1.0/conanfile.py

Co-authored-by: Uilian Ries <uilianries@gmail.com>

conan-center-bot · 2020-12-08T15:11:26Z

All green in build 12 (f1b4e53c318419f60bddce35472ee8d35ba2d9df)! 😊

logr/0.1.0: Generated 1 packages. All logs here

Co-authored-by: Uilian Ries <uilianries@gmail.com>

conan-center-bot · 2020-12-08T15:18:25Z

All green in build 14 (6750c8fcca8aee1e46d16b5c8c9f6294fbce687b)! 😊

logr/0.1.0: Generated 1 packages. All logs here

* master: (73 commits) (conan-io#3878) opengl: support FreeBSD (conan-io#3882) [system-cci] Fix hooks (conan-io#3881) [szip] Fix hooks (conan-io#3880) [tcl] Fix hooks (conan-io#3866) xorg: add freebsd packages (conan-io#3825) Adds baikal-p7 5.6 (conan-io#3758) minimal recipe for pciutils/3.7.0 (conan-io#3211) added recipe for extra-cmake-modules (conan-io#3870) tl: fix compiler version check again (conan-io#3865) bump eigen/3.3.9 (conan-io#3864) add functionalplus/0.2.13-p0 bump fmt/7.1.3 (conan-io#3694) (conan-io#3858) fix: zulu-openjdk package info (conan-io#3836) gmp: bump + enable building static libraries on MSVC (conan-io#3812) Add logr v0.1.0 (conan-io#3786) add Open62541 recipe v1.0.3 and v1.1.3 (conan-io#3696) openh264: refactor + add 2.1.1 release (conan-io#3607) add botan/2.17.2 (conan-io#3854) Add PEGTL 3.0.0 (conan-io#3859) bump spdlog/1.8.2 ...

See details here: conan-io/conan-center-index#3812 It is honoured to make it possible to package a library without extra CMake wrapper.

* Adjustments for conan packaging See details here: conan-io/conan-center-index#3812 It is honoured to make it possible to package a library without any extra CMake wrapper.

ngrodzitski added 2 commits December 6, 2020 22:09

Add a recepie for log library (v0.1.0)

edbe6f6

Add configure() method to conanfile.

9bdaddf

madebr suggested changes Dec 6, 2020

View reviewed changes

ngrodzitski and others added 3 commits December 6, 2020 22:55

Update recipes/logr/0.1.0/conanfile.py

4fd9803

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Update recipes/logr/0.1.0/conanfile.py

7299203

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Change all uses of single quotes with double quotes.

43171e1

ngrodzitski and others added 2 commits December 6, 2020 23:02

Update recipes/logr/0.1.0/test_package/conanfile.py

91739d8

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Remove package_info() method.

8f11c5b

prince-chrismc mentioned this pull request Dec 6, 2020

add cpr/1.5.2 + fix options #3813

Merged

4 tasks

conan-center-bot added the Unexpected Error label Dec 6, 2020

prince-chrismc previously approved these changes Dec 6, 2020

View reviewed changes

Update recipes/logr/0.1.0/test_package/CMakeLists.txt

24d586c

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

ngrodzitski dismissed prince-chrismc’s stale review via 24d586c December 6, 2020 20:18

conan-center-bot removed the Unexpected Error label Dec 6, 2020

prince-chrismc reviewed Dec 7, 2020

View reviewed changes

recipes/logr/0.1.0/conanfile.py Outdated Show resolved Hide resolved

prince-chrismc reviewed Dec 7, 2020

View reviewed changes

Update recipes/logr/0.1.0/conanfile.py

a00beb0

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

ngrodzitski added 2 commits December 7, 2020 18:51

Rethink options.

06c8d8c

Remove unused and duplicate cmake defs.

4db869d

fixup

ece574b

prince-chrismc reviewed Dec 8, 2020

View reviewed changes

prince-chrismc previously approved these changes Dec 8, 2020

View reviewed changes

uilianries requested changes Dec 8, 2020

View reviewed changes

recipes/logr/0.1.0/conanfile.py Show resolved Hide resolved

recipes/logr/0.1.0/conanfile.py Outdated Show resolved Hide resolved

recipes/logr/0.1.0/conanfile.py Outdated Show resolved Hide resolved

Update recipes/logr/0.1.0/conanfile.py

f1b4e53

Co-authored-by: Uilian Ries <uilianries@gmail.com>

ngrodzitski dismissed prince-chrismc’s stale review via f1b4e53 December 8, 2020 15:06

ngrodzitski and others added 2 commits December 8, 2020 18:13

Update recipes/logr/0.1.0/conanfile.py

e280a7e

Co-authored-by: Uilian Ries <uilianries@gmail.com>

Update recipes/logr/0.1.0/conanfile.py

6750c8f

Co-authored-by: Uilian Ries <uilianries@gmail.com>

uilianries approved these changes Dec 8, 2020

View reviewed changes

prince-chrismc approved these changes Dec 8, 2020

View reviewed changes

SSE4 approved these changes Dec 13, 2020

View reviewed changes

conan-center-bot merged commit 264a2e3 into conan-io:master Dec 14, 2020

ngrodzitski deleted the add-logr-v0.1.0 branch December 14, 2020 11:01

ngrodzitski added a commit to ngrodzitski/logr that referenced this pull request Dec 18, 2020

Adjustments for conan packaging

37e6e9c

See details here: conan-io/conan-center-index#3812 It is honoured to make it possible to package a library without extra CMake wrapper.

ngrodzitski mentioned this pull request Dec 18, 2020

Add logr v0.2.0 #3952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logr v0.1.0 #3812

Add logr v0.1.0 #3812

ngrodzitski commented Dec 6, 2020

CLAassistant commented Dec 6, 2020 •

edited

Loading

conan-center-bot commented Dec 6, 2020

madebr Dec 6, 2020

ngrodzitski Dec 6, 2020

madebr Dec 6, 2020

ngrodzitski Dec 6, 2020

madebr Dec 6, 2020

ngrodzitski Dec 6, 2020 •

edited

Loading

madebr Dec 6, 2020

madebr Dec 6, 2020

ngrodzitski Dec 6, 2020 •

edited

Loading

madebr Dec 6, 2020

prince-chrismc Dec 7, 2020

ngrodzitski Dec 7, 2020

conan-center-bot commented Dec 6, 2020

conan-center-bot commented Dec 6, 2020

conan-center-bot commented Dec 6, 2020

prince-chrismc Dec 6, 2020

ngrodzitski Dec 6, 2020

conan-center-bot commented Dec 6, 2020

prince-chrismc Dec 7, 2020

ngrodzitski Dec 8, 2020

conan-center-bot commented Dec 7, 2020

prince-chrismc commented Dec 7, 2020

ericLemanissier commented Dec 7, 2020

prince-chrismc commented Dec 7, 2020

conan-center-bot commented Dec 7, 2020

conan-center-bot commented Dec 7, 2020

prince-chrismc Dec 8, 2020

prince-chrismc left a comment

uilianries left a comment

conan-center-bot commented Dec 8, 2020

conan-center-bot commented Dec 8, 2020

Add logr v0.1.0 #3812

Add logr v0.1.0 #3812

Conversation

ngrodzitski commented Dec 6, 2020

CLAassistant commented Dec 6, 2020 • edited Loading

conan-center-bot commented Dec 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrodzitski Dec 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrodzitski Dec 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conan-center-bot commented Dec 6, 2020

conan-center-bot commented Dec 6, 2020

conan-center-bot commented Dec 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conan-center-bot commented Dec 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conan-center-bot commented Dec 7, 2020

prince-chrismc commented Dec 7, 2020

ericLemanissier commented Dec 7, 2020

prince-chrismc commented Dec 7, 2020

conan-center-bot commented Dec 7, 2020

conan-center-bot commented Dec 7, 2020

Choose a reason for hiding this comment

prince-chrismc left a comment

Choose a reason for hiding this comment

uilianries left a comment

Choose a reason for hiding this comment

conan-center-bot commented Dec 8, 2020

conan-center-bot commented Dec 8, 2020

CLAassistant commented Dec 6, 2020 •

edited

Loading

ngrodzitski Dec 6, 2020 •

edited

Loading

ngrodzitski Dec 6, 2020 •

edited

Loading