Adding openvdb 8.0.1 #5257

AlvaroFS · 2021-04-18T12:54:03Z

Specify library name and version: openvdb/8.0.1

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.

madebr · 2021-04-18T17:51:34Z

recipes/openvdb/8.0.1/conanfile.py

+        "shared": [True, False],
+        "fPIC": [True, False],
+        "use_blosc": [True, False],
+        "use_zlib": [True, False],
+        "use_log4cplus": [True, False],
+        "use_exr": [True, False],
+        "simd": ["None", "SSE42", "AVX"],


This repo has a (soft) convention to use with_XXX when it causes an extra dependency to be included.
e.g. with_zlib, with_log4cplus, ...
The simd is ok as it is a feature.

OK. I was just naming them as defined by the library

I understand.
Using option names that correspond with those of other packages makes the use of conan more consistent.

It's easier to tell users to use -o *:with_zlib=False to disable zlib support then to tell: -o lib1:enable_zlib=False -o lib2:with_zlib=False -o lib3:zlib=False ....
I hope I made my point 😃

Yes, I understand. I had already changed it for the next commit

madebr · 2021-04-18T17:53:17Z

recipes/openvdb/8.0.1/conanfile.py

+        if self.options.shared:
+            del self.options.fPIC
+        if self.settings.compiler.cppstd:
+            tools.check_min_cppstd(self, 14)


Suggested change

tools.check_min_cppstd(self, 14)

tools.check_min_cppstd(self, 14)

if self.settings.arch not in ("x86", "x86_64"):

if self.options.simd != "None":

raise ConanInvalidConfiguration("Only intel architectures support SSE4 or AVX.")

madebr · 2021-04-18T17:53:33Z

recipes/openvdb/8.0.1/conanfile.py

@@ -0,0 +1,250 @@
+import os
+
+from conans import ConanFile, CMake, tools


Suggested change

from conans import ConanFile, CMake, tools

from conans import ConanFile, CMake, tools

from conans.errors import ConanInvalidConfiguration

madebr · 2021-04-18T17:54:18Z

recipes/openvdb/8.0.1/conanfile.py

+            tools.check_min_cppstd(self, 14)
+
+    def requirements(self):
+        self.requires("boost/1.69.0")  # should be 1.66.0 but not in conan center


It's ok to aim higher.
When this package would be built manually on a recent linux distribution, a recent boost version would get used as well.

Suggested change

self.requires("boost/1.69.0") # should be 1.66.0 but not in conan center

self.requires("boost/1.75.0")

I was following their recommendations

I think you should interpret that table as:
"we have tested our library with this version and guarantee it to work"

As long as the library doesn't use any non-public or unstable features, I think we're ok with bumping the versions.

madebr · 2021-04-18T17:54:44Z

recipes/openvdb/8.0.1/conanfile.py

+
+    def requirements(self):
+        self.requires("boost/1.69.0")  # should be 1.66.0 but not in conan center
+        self.requires("tbb/2019_u9")  # should be 2018 but not in conan center


Suggested change

self.requires("tbb/2019_u9") # should be 2018 but not in conan center

self.requires("tbb/2020.3")

madebr · 2021-04-18T17:56:18Z

recipes/openvdb/8.0.1/conanfile.py

+        if self.options.use_blosc:
+            self.requires("c-blosc/[>1.5.0]")
+        if self.options.use_log4cplus:
+            self.requires("log4cplus/[>1.1.2]")


Suggested change

self.requires("log4cplus/[>1.1.2]")

self.requires("log4cplus/2.0.5")

madebr · 2021-04-18T17:57:12Z

recipes/openvdb/8.0.1/conanfile.py

+            "  find_package(Blosc ${MINIMUM_BLOSC_VERSION} REQUIRED)",
+            "  find_package(c-blosc ${MINIMUM_BLOSC_VERSION} REQUIRED)",


I have doubts this renaming is enough.

In a patch, you can create a proxy FindBlosc.cmake package that diverts maps the expected variabled/targets to to the one provided by conan's Findc-blosc.cmake.

I am not sure either but that sounds like a better option than renaming some of the targets to match the ones set by conan

madebr · 2021-04-18T17:57:41Z

recipes/openvdb/8.0.1/conanfile.py

+        self._cmake.definitions["Boost_USE_STATIC_LIBS"] = not self.options[
+            "boost"
+        ].shared


Suggested change

self._cmake.definitions["Boost_USE_STATIC_LIBS"] = not self.options[

"boost"

].shared

self._cmake.definitions["Boost_USE_STATIC_LIBS"] = not self.options["boost"].shared

I'm using Black with a 79 characters limit per line. Is it correct?

It looks like we all have wide screens 😉

conan-center-index/.editorconfig

Line 12 in c8fa943

max_line_length = 200

madebr · 2021-04-18T17:57:58Z

recipes/openvdb/8.0.1/conanfile.py

+        self._cmake.definitions["OPENEXR_USE_STATIC_LIBS"] = not self.options[
+            "openexr"
+        ].shared


Suggested change

self._cmake.definitions["OPENEXR_USE_STATIC_LIBS"] = not self.options[

"openexr"

].shared

self._cmake.definitions["OPENEXR_USE_STATIC_LIBS"] = not self.options["openexr"].shared

madebr · 2021-04-18T17:59:32Z

recipes/openvdb/8.0.1/conanfile.py

+        ].shared
+
+        if self.settings.os == "Windows":
+            self._cmake.definitions["OPENVDB_DISABLE_BOOST_IMPLICIT_LINKING"] = False


Does implicit linking mean use of #pragma lib(...)?
If so, then I think it's better to disable this because the name of the boost library provided by the conan package may different then the name hardcoded in the openvdb sources.

I think so. I only set it to false because it's the default value in the cmake file but I agree that it's better to disable it.

Actually I think I had it all wrong from the cmake file 😅

recipes/openvdb/8.0.1/conanfile.py

uilianries · 2021-05-07T12:03:05Z

recipes/openvdb/8.0.1/conanfile.py

+        with open("FindBlosc.cmake", "w") as f:
+            f.write(
+                """find_package(c-blosc)
+if(c-blosc_FOUND)
+    add_library(blosc INTERFACE)
+    target_link_libraries(blosc INTERFACE c-blosc::c-blosc)
+    add_library(Blosc::blosc ALIAS blosc)
+endif()
+"""
+            )


Suggested change

with open("FindBlosc.cmake", "w") as f:

f.write(

"""find_package(c-blosc)

if(c-blosc_FOUND)

add_library(blosc INTERFACE)

target_link_libraries(blosc INTERFACE c-blosc::c-blosc)

add_library(Blosc::blosc ALIAS blosc)

endif()

"""

)

with open("FindBlosc.cmake", "w") as f:

f.write(

"""find_package(c-blosc)

if(c-blosc_FOUND)

add_library(blosc INTERFACE)

target_link_libraries(blosc INTERFACE c-blosc::c-blosc)

add_library(Blosc::blosc ALIAS blosc)

endif()

"""

)

Did you try cmake_find_package generator instead?

Yes, I did, but package names are different so instead of patching the CMakelists I wrapped the conan libraries with the expected name as @madebr suggested here

Co-authored-by: Uilian Ries <[email protected]>

conan-center-bot · 2021-05-07T16:05:59Z

All green in build 7 (f8af7fbd29499594eaa6bdbde993f76089d614c9):

openvdb/8.0.1@:
All packages built successfully! (All logs)

prince-chrismc · 2021-05-16T22:41:40Z

recipes/openvdb/8.0.1/conanfile.py

+        if self.settings.os == "Windows":
+            del self.options.fPIC
+
+    def _check_compilier_version(self):


Fro my tracking conan-io/conan#8002

prince-chrismc · 2021-05-16T22:44:50Z

recipes/openvdb/8.0.1/conanfile.py

+        self._check_compilier_version()
+
+    def requirements(self):
+        self.requires("boost/1.75.0")


Can we bump to 76?

I think so. I'll change it.

Suggested change

self.requires("boost/1.75.0")

self.requires("boost/1.76.0")

Adding openvdb 8.0.1

8e454b9

AlvaroFS mentioned this pull request Apr 18, 2021

Conan package AcademySoftwareFoundation/openvdb#664

Open

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Apr 18, 2021

madebr reviewed Apr 18, 2021

View reviewed changes

Alvaro Fernandez added 3 commits April 18, 2021 23:45

Check minimum compiler version

1febda3

Update recipe with PR comments

c2dcbe5

Map conan targets from Find.cmake

994240d

This comment has been minimized.

Sign in to view

Set minimum CMake version

74dff2c

This comment has been minimized.

Sign in to view

Adapt finddXXX for older cmake versions

c0b8be3

This comment has been minimized.

Sign in to view

Set C++ version for test

876e09a

This comment has been minimized.

Sign in to view

conan-center-bot added the Unexpected Error label May 7, 2021

uilianries reviewed May 7, 2021

View reviewed changes

Apply suggestions from code review

4cac6f4

Co-authored-by: Uilian Ries <[email protected]>

This comment has been minimized.

Sign in to view

conan-center-bot removed Unexpected Error Failed labels May 7, 2021

Fix version comparison

f8af7fb

conan-center-bot requested review from memsharded and SSE4 May 10, 2021 11:59

prince-chrismc reviewed May 16, 2021

View reviewed changes

prince-chrismc approved these changes May 16, 2021

View reviewed changes

github-actions bot mentioned this pull request May 16, 2021

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

Open

SSE4 approved these changes May 17, 2021

View reviewed changes

conan-center-bot requested a review from uilianries May 17, 2021 12:00

danimtb approved these changes May 19, 2021

View reviewed changes

danimtb merged commit 630ce84 into conan-io:master May 19, 2021

SpaceIm mentioned this pull request May 25, 2021

[request] some of most relevant c++ libraries in github (by number of stars) #621

Open

AlvaroFS mentioned this pull request Aug 3, 2021

Add openvdb 8.1.0 #6127

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding openvdb 8.0.1 #5257

Adding openvdb 8.0.1 #5257

AlvaroFS commented Apr 18, 2021

This comment has been minimized.

madebr Apr 18, 2021

AlvaroFS Apr 18, 2021

madebr Apr 18, 2021

AlvaroFS Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

AlvaroFS Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

AlvaroFS Apr 18, 2021

madebr Apr 18, 2021

AlvaroFS Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

madebr Apr 18, 2021

AlvaroFS Apr 18, 2021

AlvaroFS Apr 18, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

uilianries May 7, 2021

AlvaroFS May 7, 2021

This comment has been minimized.

conan-center-bot commented May 7, 2021

prince-chrismc May 16, 2021 •

edited

Loading

prince-chrismc May 16, 2021

AlvaroFS May 17, 2021

prince-chrismc May 18, 2021

-            tools.check_min_cppstd(self, 14)
+            tools.check_min_cppstd(self, 14)
+        if self.settings.arch not in ("x86", "x86_64"):
+            if self.options.simd != "None":
+                raise ConanInvalidConfiguration("Only intel architectures support SSE4 or AVX.")

		@@ -0,0 +1,250 @@
		import os

		from conans import ConanFile, CMake, tools

	from conans import ConanFile, CMake, tools
	from conans import ConanFile, CMake, tools
	from conans.errors import ConanInvalidConfiguration

	self.requires("boost/1.69.0") # should be 1.66.0 but not in conan center
	self.requires("boost/1.75.0")

	self.requires("tbb/2019_u9") # should be 2018 but not in conan center
	self.requires("tbb/2020.3")

	self.requires("log4cplus/[>1.1.2]")
	self.requires("log4cplus/2.0.5")

		" find_package(Blosc ${MINIMUM_BLOSC_VERSION} REQUIRED)",
		" find_package(c-blosc ${MINIMUM_BLOSC_VERSION} REQUIRED)",

Adding openvdb 8.0.1 #5257

Adding openvdb 8.0.1 #5257

Conversation

AlvaroFS commented Apr 18, 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

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

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

conan-center-bot commented May 7, 2021

prince-chrismc May 16, 2021 • 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

prince-chrismc May 16, 2021 •

edited

Loading