feat: Add libbasisu package 1.15.0 #6468

mmatisko · 2021-07-23T09:35:22Z

Specify library name and version: libbasisu/1.15.0

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!

I'm not the author of this library, but it's is a dependency of for library we use and we would like to provide a Conan package for the simple use of anyone in the community.

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-07-23T09:35:27Z

All committers have signed the CLA.

madebr · 2021-07-23T13:36:33Z

recipes/libbasisu/all/conanfile.py

+        "sse": [True, False],
+        "x64": [True, False],
+        "zstd": [True, False],
+        "no_iterator_debug_level": [True, False]


Please rename this option such no double negations are possible. e.g. iterator_debug_level (and negate all tests, depending on it).

Renamed to custom_iterator_debug_level, I believe it's more readable and understandable now.

madebr · 2021-07-23T13:37:17Z

recipes/libbasisu/all/conanfile.py

+        "sse": [True, False],
+        "x64": [True, False],
+        "zstd": [True, False],
+        "no_iterator_debug_level": [True, False]


I suppose this option sets _ITERATOR_DEBUG_LEVEL.
This means that this option is only relevant for Visual Studio.
So please remove this option in config_options when self.settings.compiler != "Visual Studio".

madebr · 2021-07-23T13:38:07Z

recipes/libbasisu/all/conanfile.py

+        self._cmake.definitions["BUILD_X64"] = self.options.x64
+        self._cmake.definitions["SSE"] = self.options.sse
+        self._cmake.definitions["ZSTD"] = self.options.zstd
+        self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = self.options.no_iterator_debug_level


When you apply https://github.com/conan-io/conan-center-index/pull/6468/files#r675570927, you need to use self.options.get_safe("no_iterator_debug_level").

Suggested change

self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = self.options.no_iterator_debug_level

self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = self.options.no_iterator_debug_level

madebr · 2021-07-23T13:39:36Z

recipes/libbasisu/all/conanfile.py

+    options = {
+        "fPIC": [True, False],
+        "shared": [True, False],
+        "sse": [True, False],


What level of sse support is this?
x86_64 demands a minimum level of sse support.
Is this for x86 support? Or is this for x86_64 with extra sse? e.g. SSE4?

It's support for SSE4.1, I renamed it to use_sse4.

madebr · 2021-07-23T13:40:20Z

recipes/libbasisu/all/conanfile.py

+        "fPIC": [True, False],
+        "shared": [True, False],
+        "sse": [True, False],
+        "x64": [True, False],


Don't create an option for a settings.
You should remove this option and use self.settings.arch == "x86_64" instead.

madebr · 2021-07-23T13:40:59Z

recipes/libbasisu/all/conanfile.py

+        "shared": [True, False],
+        "sse": [True, False],
+        "x64": [True, False],
+        "zstd": [True, False],


When it means that an option will add support for an extra dependency, we usually add the with_ prefix.

Suggested change

"zstd": [True, False],

"with_zstd": [True, False],

madebr · 2021-07-23T13:41:21Z

recipes/libbasisu/all/conanfile.py

+        "zstd": True,
+        "no_iterator_debug_level": True
+    }
+    short_paths = True


Just checking, is this really required?

madebr · 2021-07-23T13:41:42Z

recipes/libbasisu/all/conanfile.py

+        if not self._minimal_compiler_version():
+            raise ConanInvalidConfiguration("{} {} does not support Visual Studio <= 14 with shared:True".format(self.name, self.version))


Please move this to a validate method.

madebr · 2021-07-23T13:42:59Z

recipes/libbasisu/all/conanfile.py

+    def build(self):
+        for patch in self.conan_data.get("patches", {}).get(self.version, []):
+            tools.patch(**patch)
+        self._clear_vs_project_files()


Is this clearing necessary? Does it bother the cmake build system?
If not, why remove it? I'ld just keep it.

madebr · 2021-07-23T13:48:24Z

recipes/libbasisu/all/conanfile.py

+        self.copy("*.h", dst=os.path.join("include", self.name, "transcoder"), src=os.path.join(self._source_subfolder, "transcoder"))
+        self.copy("*.h", dst=os.path.join("include", self.name, "encoder"), src=os.path.join(self._source_subfolder, "encoder"))


Since you're already patching the cmake script,
consider adding this instead of the call to self.copy("*.h").

install(DIRECTORY transcoder DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/libbasisu" FILES_MATCHING "*.h") install(DIRECTORY encoder DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/libbasisu" FILES_MATCHING "*.h")

or the following, if I screwed up the folders.

install(DIRECTORY transcoder DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/libbasisu/transcoder" FILES_MATCHING "*.h") install(DIRECTORY encoder DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/libbasisu/encoder" FILES_MATCHING "*.h")

Since I only wanted to make necessary changes in the cmake script, I have moved all of the installation steps to the package method in conanfile.py. But the second proposal would be ok as well.

madebr · 2021-07-23T13:49:43Z

recipes/libbasisu/all/patches/cmakelist_build_lib.patch

+
+-if (NOT MSVC)
+-   target_link_libraries(basisu m pthread)
+if (MSVC)


I think you can change this to WIN32. mingw also needs to export the symbols.

madebr · 2021-07-23T14:07:34Z

recipes/libbasisu/all/conanfile.py

+
+    def _minimal_compiler_version(self) -> bool:
+        version = tools.Version(self.settings.compiler.version.value)
+        return (self.settings.compiler == "Visual Studio" and version >= "15") \


Consider using a dict for this, it makes cleaner code. See other recipes on cci on how this can be done.
Also, unknown compiler should be allowed. A warning message can be printed then.

Resolve PR comments

madebr · 2021-07-28T11:46:01Z

recipes/libbasisu/all/patches/cmakelist_build_lib.patch

+--- a/CMakeLists.txt
+++ b/CMakeLists.txt
+@@ -1,14 +1,14 @@
+-project(basisu)


As a general note, when you see a poor cmake script that needs to be patched (heavily).
It's always nice to also send the changes to the project itself. That way, upstream benefits too from your work.

uilianries · 2021-07-28T13:38:57Z

recipes/libbasisu/all/conanfile.py

+        if self.settings.compiler.get_safe("cppstd"):
+            tools.check_min_cppstd(self, 11)


Suggested change

if self.settings.compiler.get_safe("cppstd"):

tools.check_min_cppstd(self, 11)

if self.settings.compiler.get_safe("cppstd"):

tools.check_min_cppstd(self, 11)

Move it to validate() method, because it's validating the cppstd.

I'm not going to argue, but in official documentation is check_min_cppstd used in configure.

Thanks for pointing it. Indeed check_min_cppstd is older than validate and the documentation is outdated now. I'll open an PR updating it.

uilianries · 2021-07-28T13:41:00Z

recipes/libbasisu/all/conanfile.py

+        custom_iterator_debug_level = self.options.get_safe("custom_iterator_debug_level", default="False")
+        self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = not custom_iterator_debug_level


Suggested change

custom_iterator_debug_level = self.options.get_safe("custom_iterator_debug_level", default="False")

self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = not custom_iterator_debug_level

self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = not self.options.get_safe("custom_iterator_debug_level", False)

Default won't be converted from String to Boolean.

May I ask you for any possible and correct solution for this? As I can see it's quite common in other recipes, e.g. self._autotools.fpic = self.options.get_safe("fPIC", True) in the libpcap recipe and little bit lower in the documentation as well:

def build(self): # Will return None if doesn't exist fpic = self.options.get_safe("fPIC") # Will return the default value if the return is None shared = self.options.get_safe("shared", default=False)

uilian suggested to change the argument of the default argument from the string "False" to the boolean False.
.get_safe tries to convert a value to the type of the default value.

Related to your question, I would do:

fpic = self.options.get_safe("fPIC", True) # Return True when fPIC is not available (it is often removed when built as a shared library)

Correct. Thanks!

Use default value of custom_iterator_debug_level in get_safe

conan-center-bot · 2021-07-28T18:18:46Z

All green in build 6 (b8c077018b2793d5f9a91db4d3fa0dae0ece9b49):

libbasisu/1.15.0@:
All packages built successfully! (All logs)

Croydon

Why was this package named libbasisu instead of basis_universal?

This looks wrong to me. There is also a command line tool which we might want to add at some point to this package

https://github.com/BinomialLLC/basis_universal#command-line-compression-tool

madebr · 2021-07-31T19:49:10Z

This library has vendored some libraries, which are available in cci.
See its README:

The encoder uses lodepng for loading and saving PNG images, which is Copyright (c) 2005-2019 Lode Vandevenne. It uses the zlib license. It also uses apg_bmp for loading BMP images, which is Copyright 2019 Anton Gerdelan. It uses the Apache 2.0 license.
The encoder optionally uses Zstandard's single source file compressor (in zstd/zstd.c) to support compressing supercompressed KTX2 files.

So in theory, lodepng and zstd should become requirements + libbasisu's build system modified to use these.

SpaceIm · 2021-07-31T21:47:06Z

I Agree with @Croydon, basis_univeral or basisu would have been more appropriate. Though, AFAIR, this project doesn't officially provide a lib anyway.

SpaceIm · 2021-07-31T21:52:39Z

recipes/libbasisu/all/patches/cmakelist_build_lib.patch

+-option(STATIC "static linking" FALSE)
+-option(SSE "SSE 4.1 support" FALSE)
+
+project(libbasisu VERSION 1.15.0 LANGUAGES CXX)


Not a good idea to prefix project name with lib, then rely on it for library target.

Then you end up with ugly liblib file name:

libbasisu/1.15.0 package(): Packaged 1 '.a' file: liblibbasisu.a

mmatisko · 2021-08-02T19:33:03Z

Why was this package named libbasisu instead of basis_universal?

This looks wrong to me. There is also a command line tool which we might want to add at some point to this package

https://github.com/BinomialLLC/basis_universal#command-line-compression-tool

We named this package as libbasisu as we were inspired by the package names libcurl which is only a library, but curl itself is a standalone tool. We use this library (as a library, not with a tool) in our projects, so we chose a package name that is prefixed with "lib" so that it can be distinguished as a library-only package. We also wanted to do as little changes as possible to have a fully functional Conan package with a library. That is also the reason why we used bundled versions of zlib and lodepng libraries in this package. With the next version of the library, we could also provide an option for building a cmd tool (probably with a renamed package name basisu), if you insist. In the future, the suggested improvements might be added (e.g. utilization of the zlib and lodepng packages).

feat: Add libbasisu package 1.15.0

09ddff1

This comment has been minimized.

Sign in to view

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

add check for minimal compiler versions

21a1186

This comment has been minimized.

Sign in to view

madebr reviewed Jul 23, 2021

View reviewed changes

Fix linking shared libraries into test package

334323b

Resolve PR comments

This comment has been minimized.

Sign in to view

Explicitly set and check c++11 compiler support

2396c31

mmatisko force-pushed the feature/add-libbasisu-package branch from a4ad4aa to 2396c31 Compare July 27, 2021 18:15

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Jul 28, 2021

SSE4 requested a review from uilianries July 28, 2021 06:21

madebr reviewed Jul 28, 2021

View reviewed changes

madebr previously approved these changes Jul 28, 2021

View reviewed changes

github-actions bot mentioned this pull request Jul 28, 2021

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

Open

uilianries reviewed Jul 28, 2021

View reviewed changes

uilianries mentioned this pull request Jul 28, 2021

Use validate instead of configure conan-io/docs#2178

Merged

Move check_min_cppstd to validate

b8c0770

Use default value of custom_iterator_debug_level in get_safe

mmatisko dismissed madebr’s stale review via b8c0770 July 28, 2021 16:27

uilianries approved these changes Jul 28, 2021

View reviewed changes

SSE4 approved these changes Jul 30, 2021

View reviewed changes

prince-chrismc approved these changes Jul 31, 2021

View reviewed changes

Croydon suggested changes Jul 31, 2021

View reviewed changes

conan-center-bot merged commit 800cfb0 into conan-io:master Jul 31, 2021

SpaceIm reviewed Jul 31, 2021

View reviewed changes

SpaceIm mentioned this pull request Aug 27, 2021

[request] basis_universal/1.15 #7087

Closed

SpaceIm mentioned this pull request Mar 14, 2022

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

Closed

	self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = self.options.no_iterator_debug_level
	self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = self.options.no_iterator_debug_level

		if not self._minimal_compiler_version():
		raise ConanInvalidConfiguration("{} {} does not support Visual Studio <= 14 with shared:True".format(self.name, self.version))

		self.copy("*.h", dst=os.path.join("include", self.name, "transcoder"), src=os.path.join(self._source_subfolder, "transcoder"))
		self.copy("*.h", dst=os.path.join("include", self.name, "encoder"), src=os.path.join(self._source_subfolder, "encoder"))

		if self.settings.compiler.get_safe("cppstd"):
		tools.check_min_cppstd(self, 11)

		custom_iterator_debug_level = self.options.get_safe("custom_iterator_debug_level", default="False")
		self._cmake.definitions["BASISU_NO_ITERATOR_DEBUG_LEVEL"] = not custom_iterator_debug_level

feat: Add libbasisu package 1.15.0 #6468

feat: Add libbasisu package 1.15.0 #6468

Conversation

mmatisko commented Jul 23, 2021 • edited Loading

CLAassistant commented Jul 23, 2021 • edited Loading

This comment has been minimized.

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

madebr Jul 23, 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

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.

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

mmatisko Jul 28, 2021 • 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 Jul 28, 2021

Croydon left a comment • edited Loading

Choose a reason for hiding this comment

madebr commented Jul 31, 2021

SpaceIm commented Jul 31, 2021 • edited Loading

SpaceIm Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

mmatisko commented Aug 2, 2021

mmatisko commented Jul 23, 2021 •

edited

Loading

CLAassistant commented Jul 23, 2021 •

edited

Loading

madebr Jul 23, 2021 •

edited

Loading

mmatisko Jul 28, 2021 •

edited

Loading

Croydon left a comment •

edited

Loading

SpaceIm commented Jul 31, 2021 •

edited

Loading

SpaceIm Jul 31, 2021 •

edited

Loading