Skip to content
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

Merged
merged 14 commits into from
Sep 13, 2024
Merged

added opus 1.5.1 #23234

merged 14 commits into from
Sep 13, 2024

Conversation

dmpriso
Copy link
Contributor

@dmpriso dmpriso commented Mar 24, 2024

Specify library name and version: opus/1.5.1

Added new opus version with support for DRED and deep PLC


@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

1 similar comment
@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@valgur valgur left a 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!

@conan-center-bot

This comment has been minimized.

@dmpriso
Copy link
Contributor Author

dmpriso commented Mar 28, 2024

I'm not used to using conan, I just made small adaptions to the given recipe in order to support opus 1.5.1.
If someone points me to the right direction, I'd happily have a look at the failed checks, if they are relevant.

@Croydon
Copy link
Contributor

Croydon commented Mar 28, 2024

@dmpriso You need to write a short comment in this issue -> #4, then you are going to be added and CI runs for your pull requests

@conan-center-bot

This comment has been minimized.

@dmpriso
Copy link
Contributor Author

dmpriso commented Apr 1, 2024

There seems to be a TLS issue when downloading the builds from xiph.org

@dmpriso
Copy link
Contributor Author

dmpriso commented Apr 22, 2024

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

@Croydon
Copy link
Contributor

Croydon commented Apr 22, 2024

Might have been a temporary thing since it only happened in a Conan v2 build. v1 had a regular build failure

@conan-center-bot

This comment has been minimized.

recipes/opus/all/conandata.yml Outdated Show resolved Hide resolved
recipes/opus/config.yml Outdated Show resolved Hide resolved
@Croydon
Copy link
Contributor

Croydon commented Apr 25, 2024

For protocol, the error is

opus/1.5.1: ERROR: Error downloading file https://downloads.xiph.org/releases/opus/opus-1.5.1.tar.gz: 'HTTPSConnectionPool(host='downloads.xiph.org', port=443): Max retries exceeded with url: /releases/opus/opus-1.5.1.tar.gz (Caused by SSLError(SSLError(1, '[SSL: TLSV1_ALERT_PROTOCOL_VERSION] tlsv1 alert protocol version (_ssl.c:1091)')))'
ERROR: opus/1.5.1: Error in source() method, line 59
	destination=self.source_folder, strip_root=True)
	ConanException: Error downloading file https://downloads.xiph.org/releases/opus/opus-1.5.1.tar.gz: 'HTTPSConnectionPool(host='downloads.xiph.org', port=443): Max retries exceeded with url: /releases/opus/opus-1.5.1.tar.gz (Caused by SSLError(SSLError(1, '[SSL: TLSV1_ALERT_PROTOCOL_VERSION] tlsv1 alert protocol version (_ssl.c:1091)')))'

downloads.xiph.org seems to support ONLY TLS 1,3, not any older versions.
ftp.osuosl.org seems to only support TLS 1.2, not any other version.

The Python 3.7 version in the docker containers is using the following OpenSSL version: OpenSSL 1.0.2g 1 Mar 2016
Printed via

import ssl
[...]
    def configure(self):
        self.output.info(ssl.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.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@danimtb
Copy link
Member

danimtb commented Sep 9, 2024

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 😄

@conan-center-bot

This comment has been minimized.

danimtb
danimtb previously approved these changes Sep 9, 2024
@danimtb danimtb self-assigned this Sep 9, 2024
}

def build_requirements(self):
if self.version == "1.5.2":
self.tool_requires("cmake/[>=3.16 <4]")
Copy link
Member

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

Comment on lines 25 to 27
"osce": [True, False],
"deep_plc": [True, False],
"dred": [True, False],
Copy link
Member

@uilianries uilianries Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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:

https://gitlab.xiph.org/xiph/opus/-/compare/v1.4...v1.5.2?from_project_id=36&straight=false#9a2aa4db38d3115ed60da621e012c0efc0172aae_368_401

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.

Comment on lines 70 to 75
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 14 (73bc849be05d27f0cdbb6984a0f24b2423f5545e):

  • opus/1.5.2:
    Built 16 packages out of 22 (All logs)

  • opus/1.3.1:
    Built 20 packages out of 22 (All logs)

  • opus/1.4:
    Built 20 packages out of 22 (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 14 (73bc849be05d27f0cdbb6984a0f24b2423f5545e):

  • opus/1.5.2:
    All packages built successfully! (All logs)

  • opus/1.4:
    All packages built successfully! (All logs)

  • opus/1.3.1:
    All packages built successfully! (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@conan-center-bot conan-center-bot merged commit a2f63da into conan-io:master Sep 13, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants