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

CMake: fix detection of SoundTouch pkgconfig file and version #4209

Merged
merged 2 commits into from Sep 7, 2021

Conversation

Be-ing
Copy link
Member

@Be-ing Be-ing commented Aug 16, 2021

I noticed this bug after copying this module to Tenacity.

@coveralls
Copy link

coveralls commented Aug 16, 2021

Pull Request Test Coverage Report for Build 1134039623

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 25.997%

Totals Coverage Status
Change from base Build 1133524699: 0.0%
Covered Lines: 20015
Relevant Lines: 76991

💛 - Coveralls

@daschuer
Copy link
Member

The version constrain does not seem to work:

-- Found SoundTouch: /usr/lib/x86_64-linux-gnu/libSoundTouch.so  
-- Linking libSoundTouch dynamically

Tested in a fresh build directory and SoundTouch 1.9.2 installed.

If I remove the QUIET flag it prints;

-- Checking for module 'soundtouch>=2.1.1'
--   Requested 'soundtouch >= 2.1.1' but version of SoundTouch is 1.9.2

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

This does not yet work as intended.

@uklotzde
Copy link
Contributor

$ pkgconf --list-all | grep -i soundtouch
soundtouch                     SoundTouch - SoundTouch is an open-source audio processing library for changing the Tempo, Pitch and Playback Rates of audio streams or files
libSoundTouch                  SoundTouch - SoundTouch is an open-source audio processing library for changing the Tempo, Pitch and Playback Rates of audio streams or files
soundtouch-1.0                 SoundTouch - SoundTouch is an open-source audio processing library for changing the Tempo, Pitch and Playback Rates of audio streams or files

@daschuer Please post the output on the affected system.

@Be-ing Did you try to use libSoundTouch instead of soundtouch?

@uklotzde
Copy link
Contributor

All 3 variants on Fedora 34:

-- Checking for module 'SoundTouch>=2.1.1'
--   Package 'SoundTouch', required by 'virtual:world', not found
-- Found SoundTouch: /usr/lib64/libSoundTouch.so  
-- Linking libSoundTouch dynamically
-- Checking for module 'soundtouch>=2.1.1'
--   Package dependency requirement 'soundtouch >= 2.1.1' could not be satisfied.
Package 'soundtouch' has version '2.0.0', required version is '>= 2.1.1'
-- Found SoundTouch: /usr/lib64/libSoundTouch.so  
-- Linking libSoundTouch dynamically
-- Checking for module 'libSoundTouch>=2.1.1'
--   Package dependency requirement 'libSoundTouch >= 2.1.1' could not be satisfied.
Package 'libSoundTouch' has version '2.0.0', required version is '>= 2.1.1'
-- Found SoundTouch: /usr/lib64/libSoundTouch.so  
-- Linking libSoundTouch dynamically

None seems to be correct.

@uklotzde
Copy link
Contributor

uklotzde commented Aug 29, 2021

The soundtouch.pc/libSoundTouch.pc files in Fedora 34 are outdated and advertise Version: 2.0.0 🙈

@uklotzde
Copy link
Contributor

The current package name SoundTouch is wrong, both soundtouch or libSoundTouch should work.

@uklotzde
Copy link
Contributor

The wrong version number is caused by an outdated configure.ac. This has been fixed for the new version 2.3.0.

@uklotzde
Copy link
Contributor

soundtouch is the official package name according to the build spec in Fedora:

# pkgconfig compat links for compat with older (API compatible) releases
# dunno why upstream keeps changing the pkgconfig name
# Update 2016-02-13: now looks like that is soundtouch.pc without version
ln -s soundtouch.pc %{buildroot}%{_libdir}/pkgconfig/libSoundTouch.pc
ln -s soundtouch.pc %{buildroot}%{_libdir}/pkgconfig/soundtouch-1.0.pc

@uklotzde
Copy link
Contributor

@daschuer
Copy link
Member

On Ubuntu Groovy I have this:

pkg-config --list-all | grep -i soundtouch
soundtouch                     SoundTouch - SoundTouch is an open-source audio processing library for changing the Tempo, Pitch and Playback Rates of audio streams or files

@daschuer
Copy link
Member

The found version is not checked, because it is not passed too find_package_handle_standard_args like here:

VERSION_VAR hidapi_VERSION

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I can confirm that the package name soundtouch is correct.

The version detection works for SoundTouch 2.1.2 and newer. It was broken for SoundTouch 2.1.0 and 2.1.1.

Unless we fix this bug on our side Fedora and any other distribution will not be able to use the system library for Mixxx and always link the bundled version statically.

@daschuer Please work with upstream Debian/Ubuntu if they ship an outdated package as we did for Fedora.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

@Be-ing Please change the target branch to 2.3. Done

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

We could also increase the required version number to 2.1.2 due to the buggy package configuration of SoundTouch 2.1.1.

@uklotzde uklotzde added this to the 2.3.1 milestone Sep 7, 2021
@uklotzde uklotzde changed the base branch from main to 2.3 September 7, 2021 02:36
@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Ubuntu 20.04: 2.1.2
Ubuntu 18.04: 1.9.2

It doesn't matter why the version detection for 18.04 fails, SoundTouch is too old anyway.

@daschuer
Copy link
Member

daschuer commented Sep 7, 2021

@daschuer Please work with upstream Debian/Ubuntu if they ship an outdated package as we did for Fedora.

The most recent Ubuntu Impish 21.10 ships SoundTouch 2.2 which is identified as 2.2 in the lower case pc file. The Name in the pc file is upper case. Both matches the upstream repro. So I it looks like everything is correct for Ubuntu.

I can confirm that only the lower case lookup works:

pkg-config soundtouch --modversion
2.1.2

I don't think we have an issue on Fedora, because even though it falsely identifies itself at 2.1.1 it still meets our requirements.

@daschuer
Copy link
Member

daschuer commented Sep 7, 2021

The pending issue here, preventing the merge, is that the version constrains in the pkg-config call is bypassed by the find_library() later call #4209 (comment)

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Requiring the broken version 2.1.1 is pointless, because it will never be found.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Ubuntu 20.04 build artifact is linked dynamically:

ldd /tmp/mixxx | grep -i SoundTouch
libSoundTouch.so.1 => not found

@daschuer So what's your issue here? The appended .1 version of the .so file that should actually be .2? Fedora packaging includes a patch for this that has been submitted upstream now and is included in SoundTouch 2.3.1.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Unfortunately 18.04 is not built even after switching the base branch.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

No issues on Fedora with SoundTouch 2.1.2:

ldd mixxx | grep SoundTouch
libSoundTouch.so.2 => /lib64/libSoundTouch.so.2 (0x00007f54ec5a4000)

@daschuer
Copy link
Member

daschuer commented Sep 7, 2021

Requiring the broken version 2.1.1 is pointless, because it will never be found.

We require soundtouch>=2.1.1which is found a s 2.1.2 or whatever, so there is no issue.

2.1.1 is our version in the lib folder. It looks odd if we require a higher version that we provide.
Once all our targets provide this or a higher version we can remove sound touch from our lib folder.

@daschuer So what's your issue here?

The issue is that requiring a version in the OPTIONAL pkg_check_modules branch is pointless if it is not also checked in the final find_package_handle_standard_args().

That's all.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Let's please merge this simple fix and then you could provide a follow-up PR.

Files installed by the soundtouch-devel package in Fedora 34:
❯ rpm -ql soundtouch-devel
/usr/include/soundtouch
/usr/include/soundtouch/BPMDetect.h
/usr/include/soundtouch/FIFOSampleBuffer.h
/usr/include/soundtouch/FIFOSamplePipe.h
/usr/include/soundtouch/STTypes.h
/usr/include/soundtouch/SoundTouch.h
/usr/include/soundtouch/soundtouch_config.h
/usr/lib64/libSoundTouch.so
/usr/lib64/pkgconfig/libSoundTouch.pc
/usr/lib64/pkgconfig/soundtouch-1.0.pc
/usr/lib64/pkgconfig/soundtouch.pc
/usr/share/aclocal/soundtouch.m4
@daschuer
Copy link
Member

daschuer commented Sep 7, 2021

We can't mege this to 2.3 because it is a regression for Ubuntu 18.04.

It is easy to make this megeable, see my comment where I have already outlined the solution:
#4209 (comment)

@Be-ing
Copy link
Member Author

Be-ing commented Sep 7, 2021

I moved the version check from the pkgconfig call, which didn't do anything because of the find_library executed in all cases, to the root CMakeLists.txt. Like hidapi, I implemented a fallback of reading the version from SoundTouch.h in case pkgconfig is not available.

@Be-ing Be-ing changed the title CMake: fix name of soundtouch pkgconfig file CMake: fix detection of SoundTouch pkgconfig file and version Sep 7, 2021
@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

CI build succeeded. @daschuer Ready now?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you. It is working as desired on Ubuntu Bionic and Groovy.

@daschuer daschuer merged commit cf0a029 into mixxxdj:2.3 Sep 7, 2021
@Be-ing Be-ing deleted the fix_soundtouch_pkgconfig branch October 1, 2021 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants