adding new package cvplot #8167

wpalfi · 2021-11-23T21:34:06Z

Specify library name and version: cvplot/1.2.2

Hi, I am the author of cvplot. We provided cvplot conan packages on bintray until sunset. Would be great to have them in conan center now.

There is a header_only option in the recipe, I am not sure if I used it correctly. E.g. conan search cvplot/1.2.2@ -q header_only=True does not find the package, but I guess that is by design.

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 · 2021-11-23T21:34:12Z

All committers have signed the CLA.

stale · 2021-12-24T11:19:11Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wpalfi · 2021-12-27T11:15:17Z

Header-only is sufficient. The library is very small and we can avoid all possible problems coming from the heavy opencv requirement.

uilianries

Thank you for bringing your project to Conan Center!

I have question about the CMake target ...
Conan will generate cvplot as target name, but your project doesn't export a target, however, it uses CvPlot (CamelCase) as internal target name. So my question is, which one is the correct to be exported?

recipes/cvplot/all/conanfile.py

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

wpalfi · 2021-12-29T19:01:45Z

Conan will generate cvplot as target name, but your project doesn't export a target, however, it uses CvPlot (CamelCase) as internal target name. So my question is, which one is the correct to be exported?

Good question! I dont know. The CMake target (and c++ namespace) is CvPlot for historical reasons (cvplot would be nicer). Conan's convention for the package name is cvplot. I guess keeping the taget name CvPlot is more consistent and should be no problem with conan. What do you think?

uilianries · 2021-12-30T09:27:47Z

@wpalfi Conan tries to follow the upstream as most important rule, we don't try to customize target names. When a project doesn't specify a target, we usually get the project name, or what's most popular on the internet, like official Findxxx.cmake or projects consuming. Otherwise, we just use lower cased name.

Said that, we should use CvPlot, because it's aligned with the upstream.

recipes/cvplot/all/conanfile.py

recipes/cvplot/all/test_package/CMakeLists.txt

recipes/cvplot/all/test_package/conanfile.py

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

recipes/cvplot/all/conanfile.py

prince-chrismc · 2022-01-05T01:24:15Z

recipes/cvplot/all/conanfile.py

+
+    def package_info(self):
+        self.cpp_info.defines.append("CVPLOT_HEADER_ONLY")
+        self.cpp_info.set_property("cmake_find_mode", "both") 


I was not able to find any modules being installed 🤔

Suggested change

self.cpp_info.set_property("cmake_find_mode", "both")

Not sure about that. I thought both means, that we support both find_package(CvPlot CONFIG) and find_package(CvPlot MODULE)?

I tried the MODULE version. Conan creates a Findcvplot.cmake then but cmake does not find opencv:

-- Found cvplot: 1.2.2 (found version "1.2.2") -- Conan: Target declared 'cvplot::cvplot' CMake Error at /usr/share/cmake-3.16/Modules/CMakeFindDependencyMacro.cmake:47 (find_package): No "Findopencv.cmake" found in CMAKE_MODULE_PATH. Call Stack (most recent call first): build/7e3f3c9f8b0319ca5c01315759aeda2f877da74f/FindCvPlot.cmake:25 (find_dependency) CMakeLists.txt:7 (find_package)

So I guess we should stick to the default config mode?

have to add

self.cpp_info.set_property("cmake_file_name", "CvPlot") self.cpp_info.set_property("cmake_target_name", "CvPlot::CvPlot")

That solves the CvPlot spelling, but I still dont know how to consume the FindCvPlot.cmake. Always getting No "Findopencv.cmake".

hmm, Conan generates OpenCV.cmake not opencv.cmake. Do you have a log? The CI is working for the test package.

I think some consumers may use find_package(cvplot CONFIG) and some may use find_package(cvplot MODULE), right? is there any reason not to support both use-cases?

@SSE4 as I said above:

find_package(cvplot MODULE) already works with generators = "cmake", "cmake_find_package" and does not need self.cpp_info.set_property("cmake_find_mode", "both")

The only problem is with generators = "cmake", "CMakeDeps" (No "Findopencv.cmake" found)

According to the CMake docs https://cmake.org/cmake/help/latest/manual/cmake-modules.7.html#find-modules find_package(cvplot MODULE) does not exist ... to my knowledge. I do not expect it to work

this list built-in CMake modules only, but someone can use the custom Find module in his own project, right? I've seen it in a lot of OSS projects in the wild, it's pretty common practice

Sadly theres been a lot of misuse... downstream project can write their own (which is fairly common) then this setting makes sense

https://cmake.org/cmake/help/latest/command/find_package.html#id4

The file CMake searches for in module mode should not be provided by the package itself.

Looking in the github repo I dont see any custom files... but it's not easy to search all projects

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

conan-center-bot · 2022-01-05T10:56:00Z

All green in build 6 (0b46e18d626b6a0e5bea2632f5f0f482a7556de7):

cvplot/1.2.2@:
All packages built successfully! (All logs)

* added cvplot * using del self.options.shared * c++11 std in test_package added * header-only only * Use MIT short name Co-authored-by: Uilian Ries <uilianries@gmail.com> * more topics Co-authored-by: Uilian Ries <uilianries@gmail.com> * CvPlot as target name Co-authored-by: Uilian Ries <uilianries@gmail.com> * dont need cmake in package recipe Co-authored-by: Chris Mc <prince.chrismc@gmail.com> Co-authored-by: Uilian Ries <uilianries@gmail.com> Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

wpalfi added 2 commits November 23, 2021 22:07

added cvplot

83753e6

using del self.options.shared

d4c1c60

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Nov 23, 2021

c++11 std in test_package added

6d1ed8e

stale bot added the stale label Dec 24, 2021

header-only only

5b21a6d

stale bot removed the stale label Dec 27, 2021

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Dec 27, 2021

prince-chrismc mentioned this pull request Dec 27, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot requested a review from uilianries December 27, 2021 12:03

uilianries reviewed Dec 28, 2021

View reviewed changes

recipes/cvplot/all/conanfile.py Outdated Show resolved Hide resolved

recipes/cvplot/all/conanfile.py Outdated Show resolved Hide resolved

conan-center-bot requested a review from SSE4 December 28, 2021 12:01

wpalfi and others added 2 commits December 29, 2021 19:43

Use MIT short name

17194b6

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

more topics

ebb54ea

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

This comment has been minimized.

Sign in to view

uilianries reviewed Dec 30, 2021

View reviewed changes

recipes/cvplot/all/conanfile.py Show resolved Hide resolved

recipes/cvplot/all/conanfile.py Outdated Show resolved Hide resolved

recipes/cvplot/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved

recipes/cvplot/all/test_package/conanfile.py Outdated Show resolved Hide resolved

CvPlot as target name

022324b

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

This comment has been minimized.

Sign in to view

prince-chrismc reviewed Jan 5, 2022

View reviewed changes

dont need cmake in package recipe

0b46e18

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

SSE4 approved these changes Jan 24, 2022

View reviewed changes

SSE4 requested a review from uilianries January 24, 2022 12:42

uilianries approved these changes Jan 24, 2022

View reviewed changes

prince-chrismc approved these changes Jan 25, 2022

View reviewed changes

conan-center-bot merged commit c704f8c into conan-io:master Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding new package cvplot #8167

adding new package cvplot #8167

wpalfi commented Nov 23, 2021

CLAassistant commented Nov 23, 2021 •

edited

Loading

This comment has been minimized.

stale bot commented Dec 24, 2021

This comment has been minimized.

wpalfi commented Dec 27, 2021

uilianries left a comment

wpalfi commented Dec 29, 2021

This comment has been minimized.

uilianries commented Dec 30, 2021

This comment has been minimized.

prince-chrismc Jan 5, 2022

wpalfi Jan 5, 2022

uilianries Jan 5, 2022

wpalfi Jan 5, 2022

uilianries Jan 5, 2022

SSE4 Jan 10, 2022

wpalfi Jan 10, 2022

prince-chrismc Jan 10, 2022 •

edited

Loading

SSE4 Jan 10, 2022

prince-chrismc Jan 13, 2022

conan-center-bot commented Jan 5, 2022

adding new package cvplot #8167

adding new package cvplot #8167

Conversation

wpalfi commented Nov 23, 2021

CLAassistant commented Nov 23, 2021 • edited Loading

This comment has been minimized.

stale bot commented Dec 24, 2021

This comment has been minimized.

wpalfi commented Dec 27, 2021

uilianries left a comment

Choose a reason for hiding this comment

wpalfi commented Dec 29, 2021

This comment has been minimized.

uilianries commented Dec 30, 2021

This comment has been minimized.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince-chrismc Jan 10, 2022 • edited Loading

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 Jan 5, 2022

CLAassistant commented Nov 23, 2021 •

edited

Loading

prince-chrismc Jan 10, 2022 •

edited

Loading