-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
added opus 1.5.1 #23234
added opus 1.5.1 #23234
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
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, only need to add CMP0077. Thanks!
This comment has been minimized.
This comment has been minimized.
I'm not used to using conan, I just made small adaptions to the given recipe in order to support opus 1.5.1. |
This comment has been minimized.
This comment has been minimized.
There seems to be a TLS issue when downloading the builds from xiph.org |
Anything I can do about this? The releases are hosted on xiph.org, but somehow the build servers don't accept the certificate or TLS version, see e.g. https://c3i.jfrog.io/c3i/misc-v2/logs/pr/23234/4-linux-gcc/opus/1.5.1//1d2e365eb8c339034b0dc24d7dd442ac92ae783a-build.txt |
Might have been a temporary thing since it only happened in a Conan v2 build. v1 had a regular build failure |
This comment has been minimized.
This comment has been minimized.
For protocol, the error is
downloads.xiph.org seems to support ONLY TLS 1,3, not any older versions. The Python 3.7 version in the docker containers is using the following OpenSSL version:
OpenSSL added TLS 1.3 support in 1.1.1+ Using the mirror directly as in my code suggestion above we should be able to workaround this problem for now. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I think this PR is ready to merge even though the CLA Assistant complains about "Daniel" not having signed it (however, I believe it is the same person as @dmpriso). Update: I have updated the first commit with the correct GitHub email of the original author to overcome the CLA situation and be able to push forward this PR 😄 |
b9193e3
to
c0c1f61
Compare
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
def build_requirements(self): | ||
if self.version == "1.5.2": | ||
self.tool_requires("cmake/[>=3.16 <4]") |
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 was required by the original cmakelist of the library
recipes/opus/all/conanfile.py
Outdated
"osce": [True, False], | ||
"deep_plc": [True, False], | ||
"dred": [True, 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.
"osce": [True, False], | |
"deep_plc": [True, False], | |
"dred": [True, False], |
I would suggest removing these new options.
When any of them is enabled, it will require DNN support as well:
Which results in the follow error:
-- Configuring done (14.7s)
CMake Error at cmake/OpusFunctions.cmake:180 (target_sources):
Cannot find source file:
dnn/fargan_data.h
Tried extensions .c .C .c++ .cc .cpp .cxx .cu .mpp .m .M .mm .ixx .cppm
.ccm .cxxm .c++m .h .hh .h++ .hm .hpp .hxx .in .txx .f .F .for .f77 .f90
.f95 .f03 .hip .ispc
Call Stack (most recent call first):
CMakeLists.txt:400 (add_sources_group)
The dnn/fargan_data.h
is a generated file, resulted from the pre-training commands using Python command, this is a pre-step, before building, something hard to be managed: https://gitlab.xiph.org/xiph/opus/-/blob/v1.5.2/dnn/torch/fargan/README.md#L37
As those options are OFF by default, even in the project (https://gitlab.xiph.org/xiph/opus/-/blob/v1.5.2/CMakeLists.txt#L86) we don't see it, but it's the scenario that will result in new issues for CCI in the future.
recipes/opus/all/conanfile.py
Outdated
tc.variables["OPUS_FIXED_POINT"] = self.options.fixed_point | ||
tc.variables["OPUS_STACK_PROTECTOR"] = self.options.stack_protector | ||
tc.variables["OPUS_OSCE"] = self.options.osce | ||
tc.variables["OPUS_DEEP_PLC"] = self.options.deep_plc | ||
tc.variables["OPUS_DRED"] = self.options.dred | ||
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW" |
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.
tc.variables["OPUS_FIXED_POINT"] = self.options.fixed_point | |
tc.variables["OPUS_STACK_PROTECTOR"] = self.options.stack_protector | |
tc.variables["OPUS_OSCE"] = self.options.osce | |
tc.variables["OPUS_DEEP_PLC"] = self.options.deep_plc | |
tc.variables["OPUS_DRED"] = self.options.dred | |
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW" | |
tc.cache_variables["OPUS_BUILD_SHARED_LIBRARY"] = self.options.shared | |
tc.cache_variables["OPUS_FIXED_POINT"] = self.options.fixed_point | |
tc.cache_variables["OPUS_STACK_PROTECTOR"] = self.options.stack_protector | |
if Version(self.version) >= "1.5.2" and is_msvc(self): | |
tc.cache_variables["OPUS_STATIC_RUNTIME"] = is_msvc_static_runtime(self) |
- Considering that those new options osce, deep_plc and dred are not exposed.
- Using cache_variables avoids CMP0077 usage
- Opus uses dynamic runtime library by default, matching with CCI always, but silently failing when is static/MT
Signed-off-by: Uilian Ries <[email protected]>
Signed-off-by: Uilian Ries <[email protected]>
Signed-off-by: Uilian Ries <[email protected]>
Signed-off-by: Uilian Ries <[email protected]>
Signed-off-by: Uilian Ries <[email protected]>
Conan v1 pipeline ✔️All green in build 14 (
Conan v2 pipeline ✔️
All green in build 14 ( |
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
Specify library name and version: opus/1.5.1
Added new opus version with support for DRED and deep PLC