-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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] Add support for Conan v2 #11981
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This reverts commit b4672c5.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
cmake_deps = CMakeDeps(self) | ||
cmake_deps.generate() |
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.
CMakeDeps not needed if header_only
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.
True!
elif tools.Version(self.version) >= "1.5.0": | ||
self.requires("fmt/6.2.1") | ||
# TODO: Remove in Conan 2.0 - 1.x self.requires does not support transitive_headers | ||
requires = lambda ref: self.requires(ref, transitive_headers=True) if Version(conan_version) >= "2.0.0-beta" else self.requires(ref) |
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 does transitive_headers=True
mean? Ok: https://docs.conan.io/en/2.0/reference/conanfile/methods.html?highlight=transitive_headers#transitive-headers, some sort of PRIVATE vs INTERFACE. It's not clear in the documentation what is the default value, nor if it's as smart as PRIVATE/PUBLIC/INTERFACE CMake properties.
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.
Indeed, I needed to ask Conan devs to understand. The fmt header is required in a public spglog header, but on the test package, fmt headers are not available by default, so to avoid the failure, we need trasitive_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.
We'll be working on the docs more and more.
Best reference as of know I think is this talk https://www.youtube.com/watch?v=kKGglzm5ous&t=2025
My understanding is it should be the "same" as CMake's at least in concept 🤞
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.
conan 1.52 will allow these arguments in self.requires
(see conan-io/conan#11934), but my understanding is that it will be a no-op in conan v1 mode (everything is always public), it's just a way to ease the migration of recipes to conan v2, since the default value of transitive_*
arguments is False.
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 pr is directly from this work #11981 (comment)
Which is why this migration is super important
Once 1.52 is available we cam clean this one up 👍
Some point are still missing:
validate
method needsself.info.options
which has support toget_safe
on Conan 2.0, but it's not supported by Conan 1.51.0. Thus, validate does not work withself.info.clear()
under thepackage_id
:validate
tovalidate_build
and, follow old fashion way, withoutself.info
, it works, but during the package, it's not able to find its targets:This same error occurs with the
fmt
package.