Add soxr library #7728

chausner · 2021-10-16T21:40:56Z

Specify library name and version: soxr/0.1.3

This adds the new library soxr.

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. (Running into conan-center hook always reports "pre_export(): could not get source code" on WIndows hooks#157)

CLAassistant · 2021-10-16T21:41:00Z

All committers have signed the CLA.

chausner · 2021-10-16T21:51:19Z

[HOOK - conan-center.py] pre_export(): ERROR: [EXPORT LICENSE (KB-H023)] This recipe is exporting a license file. Remove licenses/** from exports_sources (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H023)

The license file that the recipe exports is not about the recipe's license but applies to one part of the library. To be exact, it has been taken from the file header in pffft.c because this file has a different license than the rest of the library. See also the note at the end in LICENCE:

If building with pffft.c, see the licence embedded in that file.

How should I handle this case?

jgsogo

You can parse the file, take those lines and create the license file on the fly

jgsogo · 2021-10-18T08:24:55Z

recipes/soxr/all/patches/findpackage-openmp.patch

+
+ if (WITH_OPENMP)
+-  find_package (OpenMP)
+  find_package (OpenMP REQUIRED)


Is it required? It is an option in the recipe, what happens if soxr:with_openmp=False?

See the comment at the top of recipes/soxr/all/patches/findpackage-openmp.patch. I am setting the "REQUIRED" flag because I want CMake to fail early if users set "with_openmp=True" and CMake cannot find the package. Otherwise, the build would succeed and later builds would fail when attempting to link to libsoxr because the compiler flags are still getting set for OpenMP in package_info.

It would be even better to check it on Conan side, the message to the user could be better and the check performed even before trying to build anything. Also, Conan can run the check even if the binary is already available, when a consumer tries to use this recipe (and the compiler flag is propagated), not just when it is being built.

Is this some library we can build from sources (we have llvm-openmp)? Is this one use-case for a openmp/system package?

It would be even better to check it on Conan side,

Yes, I agree it would be better to check on the Conan side. But I don't think there is a reliable way to check for the presence of a CMake package from within Conan (without invoking CMake itself), is there? What do you suggest in this case?

Is this some library we can build from sources (we have llvm-openmp)? Is this one use-case for a openmp/system package?

Hmm, normally OpenMP is not built from sources. On Linux and macOS, it's commonly installed as a system package. On Windows, it comes with Visual Studio. I am not familiar with llvm-openmp but I doubt this would be the package you normally would want to use on Windows, for example.

It would be even better to check it on Conan side,

Yes, I agree it would be better to check on the Conan side. But I don't think there is a reliable way to check for the presence of a CMake package from within Conan (without invoking CMake itself), is there? What do you suggest in this case?

The build system should follow instructions from the package-manager. If the package manager is configured with with_openmp=True, it should pass the option to the build-system and this one should find and use it or fail (the REQUIRED is a good thing). We try to avoid auto-detection as much as possible, because it makes the process uncertain and not reproducible.

If OpenMP is expected to be in the system, then the best way to go is to create a system package, Conan will check if the package is available in the system (using some underlying tool like pkg_config) and generate the proper FindOpenMP.cmake to use it from system libraries.... or fail if the package is required and not available.

It would work like any other requirement.

(the REQUIRED is a good thing)

Okay, yes I agree and might have misunderstood you here.

If OpenMP is expected to be in the system, then the best way to go is to create a system package,

I see. Unfortunately, I will not be able to contribute that one. I am surprised it does not exist yet, as there should be more existing packages in Conan Center that depend on OpenMP in some way.

For now, I suggest to either go with the current solution in this PR, or if you prefer, I can also completely remove the with_openmp option.

I see there are other packages in ConanCenter with a with_openmp option and all of them default to False, like yours, and it is ok. We can leave it as it is, probably it is something to be addressed in the future and, at that point, we can improve all the recipes.

My initial concern about this issue was if creating (and maintaining) a patch to add the REQUIRED keyword was worth it or not, because it is the only patch in this recipe and the only line changed.

This change looks fine to me.

chausner · 2021-10-18T22:08:45Z

You can parse the file, take those lines and create the license file on the fly

Done in 1181965.

chausner · 2021-10-19T14:27:17Z

Not sure why but I had to set the fPIC option explicitly as done in some of the other ports, otherwise it would not apply the option:

self._cmake.definitions["CMAKE_POSITION_INDEPENDENT_CODE"] = self.options.get_safe("fPIC", True)

uilianries · 2021-10-19T16:45:34Z

Not sure why but I had to set the fPIC option explicitly as done in some of the other ports, otherwise it would not apply the option:

self._cmake.definitions["CMAKE_POSITION_INDEPENDENT_CODE"] = self.options.get_safe("fPIC", True)

Not needed if you are using CMake helper. It already manages fPIC automatically when the option fPIC is available.

uilianries

No cmake wrapper? Please, use https://github.com/conan-io/conan-center-index/blob/master/recipes/szip/all/CMakeLists.txt as reference.

Thank you for your contribution!

recipes/soxr/all/conanfile.py

uilianries · 2021-10-19T16:50:09Z

recipes/soxr/all/conanfile.py

+        cmake.build()
+
+    def _extract_pffft_license(self):
+        # extract license header from pffft.c and store it in the package folder


It can be very simplified: https://docs.conan.io/en/latest/howtos/extract_licenses_from_headers.html

Thanks for the pointer. I have rewritten it using conans.tools and regex: 5865a82

recipes/soxr/all/test_package/CMakeLists.txt

recipes/soxr/all/test_package/conanfile.py

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

uilianries

Please, add the cmake wrapper

chausner · 2021-10-22T18:35:22Z

Please, add the cmake wrapper

I added the wrapper in 375aca0.

recipes/soxr/all/conanfile.py

madebr · 2021-10-23T15:25:15Z

recipes/soxr/all/patches/findpackage-openmp.patch

+
+ if (WITH_OPENMP)
+-  find_package (OpenMP)
+  find_package (OpenMP REQUIRED)


This change looks fine to me.

recipes/soxr/all/conanfile.py

madebr · 2021-10-23T15:33:16Z

recipes/soxr/all/conanfile.py

+        "shared": [True, False], 
+        "fPIC": [True, False], 
+        "with_openmp": [True, False],
+        "with_lsr_bindings": [True, False]


This option will create a soxr-lsr library (see bottom of this cmake script)
It will also create a soxr-lsr.pc file.

So you will probably need to use modules in package_info.

I split it up into two components "core" and "lsr". Please have a look: fc13703

LGTM, thanks for your contribution!

recipes/soxr/all/conanfile.py

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

conan-center-bot · 2021-10-23T20:49:04Z

All green in build 15 (8b969f092c608db54a1088e4f9d42f3c59c887fb):

soxr/0.1.3@:
All packages built successfully! (All logs)

uilianries

LGTM

* Add soxr port * Make config.yml end with a newline * Extract pffft license header from source file * Remove pkgconfig and share folders * Add libm to package_info * Fix fPIC option * Use cmake_find_package_multi generator Co-authored-by: Uilian Ries <uilianries@gmail.com> * Simplify license header extraction * Minor tweak * Fix tools.save call * Use CMake conan helper wrapper * Fix missing newline at end-of-file * Apply suggestions from code review Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com> * Add separate components for core and lsr * Fix missing newline at end-of-file Co-authored-by: chausner <chausner@users.noreply.github.com> Co-authored-by: Uilian Ries <uilianries@gmail.com> Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Add soxr port

d6762b3

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 16, 2021

Make config.yml end with a newline

0aaf9b5

This comment has been minimized.

Sign in to view

jgsogo reviewed Oct 18, 2021

View reviewed changes

Extract pffft license header from source file

1181965

This comment has been minimized.

Sign in to view

Remove pkgconfig and share folders

35e59ad

This comment has been minimized.

Sign in to view

Add libm to package_info

65b9599

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Oct 18, 2021

prince-chrismc mentioned this pull request Oct 19, 2021

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

Open

conan-center-bot requested a review from uilianries October 19, 2021 12:03

Fix fPIC option

2d46b91

This comment has been minimized.

Sign in to view

uilianries reviewed Oct 19, 2021

View reviewed changes

chausner and others added 2 commits October 19, 2021 19:35

Use cmake_find_package_multi generator

49f7167

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

Simplify license header extraction

5865a82

This comment has been minimized.

Sign in to view

conan-center-bot added the Unexpected Error label Oct 19, 2021

Minor tweak

db2a475

This comment has been minimized.

Sign in to view

Fix tools.save call

43a9c26

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Oct 19, 2021

conan-center-bot requested a review from uilianries October 22, 2021 12:03

uilianries reviewed Oct 22, 2021

View reviewed changes

Use CMake conan helper wrapper

375aca0

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 22, 2021

Fix missing newline at end-of-file

ba312c9

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Oct 22, 2021

madebr reviewed Oct 23, 2021

View reviewed changes

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

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

madebr reviewed Oct 23, 2021

View reviewed changes

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

Apply suggestions from code review

d59c954

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

This comment has been minimized.

Sign in to view

Add separate components for core and lsr

fc13703

This comment has been minimized.

Sign in to view

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

Fix missing newline at end-of-file

8b969f0

conan-center-bot removed the Failed label Oct 23, 2021

madebr approved these changes Oct 23, 2021

View reviewed changes

uilianries approved these changes Oct 25, 2021

View reviewed changes

SSE4 approved these changes Oct 25, 2021

View reviewed changes

conan-center-bot merged commit d298dc7 into conan-io:master Oct 25, 2021

chausner deleted the soxr branch October 26, 2021 22:52

SpaceIm mentioned this pull request Mar 14, 2022

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

Closed

Add soxr library #7728

Add soxr library #7728

Conversation

chausner commented Oct 16, 2021

CLAassistant commented Oct 16, 2021 • edited Loading

This comment has been minimized.

chausner commented Oct 16, 2021 • edited Loading

This comment has been minimized.

jgsogo left a 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

chausner commented Oct 18, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chausner commented Oct 19, 2021

This comment has been minimized.

uilianries commented Oct 19, 2021

uilianries left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chausner Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

uilianries left a comment

Choose a reason for hiding this comment

chausner commented Oct 22, 2021

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

madebr Oct 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

This comment has been minimized.

This comment has been minimized.

conan-center-bot commented Oct 23, 2021

uilianries left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 16, 2021 •

edited

Loading

chausner commented Oct 16, 2021 •

edited

Loading

chausner Oct 19, 2021 •

edited

Loading

madebr Oct 23, 2021 •

edited

Loading