Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add soxr library #7728
Add soxr library #7728
Changes from 12 commits
d6762b3
0aaf9b5
1181965
35e59ad
65b9599
2d46b91
49f7167
5865a82
db2a475
43a9c26
375aca0
ba312c9
d59c954
fc13703
8b969f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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
.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 split it up into two components "core" and "lsr". Please have a look: fc13703
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, thanks for your contribution!
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 can be very simplified: https://docs.conan.io/en/latest/howtos/extract_licenses_from_headers.html
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.
Thanks for the pointer. I have rewritten it using
conans.tools
and regex: 5865a82There 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.
Is it required? It is an option in the recipe, what happens if
soxr:with_openmp=False
?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.
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
.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 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 aopenmp/system
package?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.
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?
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.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 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 (theREQUIRED
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.
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.
Okay, yes I agree and might have misunderstood you here.
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.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 see there are other packages in ConanCenter with a
with_openmp
option and all of them default toFalse
, 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.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 change looks fine to me.