-
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
polymorphic_value: conan v2 support #13973
Conversation
This comment has been minimized.
This comment has been minimized.
done as part of @prince-chrismc's webinar
8402928
to
55c8f45
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thank so much for joining!!!
Great work here 🎉
Just a few tiny comments
else: | ||
if tools.Version(self.settings.compiler.version) < min_version: | ||
if tools.Version(self.info.settings.compiler.version) < min_version: |
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 is in conan.tools.scm
:)
self.info.clear() | ||
|
||
def layout(self): | ||
basic_layout(self, src_folder="source") |
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 is just a style convention, 😎
basic_layout(self, src_folder="source") | |
basic_layout(self, src_folder="src") |
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.
You might want to have that in the docs then: https://docs.conan.io/en/latest/migrating_to_2.0/recipes.html#the-layout-method
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.
@dvirtz Conan docs is different from ConanCenterIndex convention. It's well documented on ConanCenterIndex templates: https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/cmake_package/all/conanfile.py#L70
|
||
required_conan_version = ">=1.43.0" | ||
from conan import ConanFile, tools |
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 is a private import, we recommend important the functions directly in CCI
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
# FIXME: workaround https://github.com/conan-io/conan/issues/12210 | ||
info = self if Version(conan_version).major < 2 else self.info |
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.
No workaround, please. It should be fixed on next releases.
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.
reverted
This reverts commit 84f85e4.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
So far is not possible to combine self.info.clear()
with self.info.settings
. Conan V2 will be adapted on Beta-5 to work with self.settings
on validate()
method.
We have some open threads where you can obtain more information:
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.
apply suggestions pressed wrong button
Co-authored-by: Uilian Ries <uilianries@gmail.com>
find_package(polymorphic_value REQUIRED CONFIG) | ||
|
||
add_executable(${PROJECT_NAME} test_package.cpp) | ||
target_link_libraries(${PROJECT_NAME} PRIVATE polymorphic_value::polymorphic_value) | ||
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_20) | ||
|
||
enable_testing() |
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 added it to conan new
but I regret, because cmake test hides the output.
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.
It saves the need to track down the executable path though
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 running it the virtualenv. Running a test from cmake command, does not guarantee which environment (conanrun, conanbuild) is being used.
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.
CMake.test()
should take the environment as an argument then
|
||
|
||
class TestPackageConan(ConanFile): | ||
settings = "os", "arch", "compiler", "build_type" | ||
generators = "cmake", "cmake_find_package_multi" | ||
generators = "CMakeDeps", "CMakeToolchain" |
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.
generators = "CMakeDeps", "CMakeToolchain" | |
generators = "CMakeDeps", "CMakeToolchain", "VirtualRunEnv" | |
test_type = "explicit" |
self.run(os.path.join("bin", "test_package"), run_environment=True) | ||
if can_run(self): | ||
cmake = CMake(self) | ||
cmake.test() |
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 correct would run self.run
which I prefer because you need to invoke with env=conanrun
and I'm not sure if cmake.test()
preserves it. So please, follow https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/cmake_package/all/test_package/conanfile.py as example which we know is safe.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipelineAll green in build 9 (
|
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.
LGTM
Specify library name and version: polymorphic_value/*
done as part of @prince-chrismc's webinar