Bug 5154 - Review request: libldac - LDAC library from AOSP
Summary: Review request: libldac - LDAC library from AOSP
Status: CLOSED FIXED
Alias: None
Product: Package Reviews
Classification: Unclassified
Component: Review Request (show other bugs)
Version: Current
Hardware: x86_64 GNU/Linux
: P1 enhancement
Assignee: RPM Fusion Package Review
URL:
Depends on:
Blocks: NEEDSPONSORS 5155
  Show dependency treegraph
 
Reported: 2019-01-30 04:45 CET by Gergely Gombos
Modified: 2020-11-13 15:51 CET (History)
4 users (show)

See Also:
namespace: free


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Gergely Gombos 2019-01-30 04:45:36 CET
Repo:
https://pagure.io/libldac

Spec:
https://pagure.io/libldac/blob/master/f/libldac.spec
SRPM:
https://pagure.io/libldac/blob/master/f/libldac-2.0.2.2-1.fc30.src.rpm

Description:
LDAC library from AOSP.
LDAC is an audio coding technology developed by Sony.
It enables the transmission of High-Resolution Audio content,
even over a Bluetooth connection.

pulseaudio-modules-bluetooth-aptx cannot be included in Fedora due to a build dependency on ffmpeg.

For compatible headsets, this package enables LDAC encoding over Bluetooth for pulseaudio-modules-bluetooth-aptx.

Rpmlint output - spec:
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Rpmlint output - binary:
libldac.x86_64: W: unstripped-binary-or-object /usr/lib64/libldacBT_abr.so.2.0.2.2
libldac.x86_64: W: unstripped-binary-or-object /usr/lib64/libldacBT_enc.so.2.0.2.2
libldac.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Please let me know if binary stripping is needed and how to do it.
The .so file names are libldacBT.so... in the source cmake files, libldac and libldacBT are both used, but IMHO the package should simply be named libldac.

This is my first RPM(Fusion) package.
I'm looking for a sponsor because of this!
Comment 1 leigh scott 2019-01-30 10:28:02 CET
Why does this package qualify to be in rpmfusion?, it requires nothing in our repo and the code looks suitable for fedora.

> pulseaudio-modules-bluetooth-aptx cannot be included in Fedora due to a
> build dependency on ffmpeg.


That doesn't qualify as a valid reason, please submit this to fedora instead.



I had a quick look anyway.


The srpm isn't usable

$ file libldac-2.0.2.2-1.fc30.src.rpm
libldac-2.0.2.2-1.fc30.src.rpm: HTML document, ASCII text


1: Please use the cmake3 macro

change

%build
# These switches are needed so that output will be in the correct directories
cmake \

to

%build
# These switches are needed so that output will be in the correct directories
%{cmake3} \


2: Remove

%global debug_package %{nil}

You probably have no debug info as your buildroot is incomplete, mock and the build servers install redhat-rpm-config package by default.


3: Remove the obsolete rm -rf %{buildroot} from

%install
rm -rf %{buildroot}


4: Remove the obsolete scriptlets

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig


5: Shorten url to

Source0:        %{url}/releases/download/v%{version}/%{zipname}-%{version}.tar.gz
Comment 2 Nicolas Chauvet 2019-01-30 10:41:36 CET
Having a quick look, it's possible that this library implement a patented algo that Sony might provide only to licensee.

But this isn't clear if this is an evil patent, so please submit for review in fedora and block fe-legal. If relevant it can still be re-open here.
Comment 3 Xavier Bachelot 2019-01-30 11:05:03 CET
Some comments :
-> The spec and SRPM URLs are incorrect. They need to point at the raw files.

%global debug_package %{nil}
-> We do want a debuginfo package. The build will take care of stripping the binaries.

%global zipname ldacBT
-> Cosmetic, but it looks weird to use zipname when the payload is a tarball.

BuildRequires:  automake
BuildRequires:  libtool
-> Not needed

-> Missing BuildRequires on gcc

%build
cmake \
..snip...
-> use %cmake. This will take care of setting most of the needed parameters and also properly set the CFLAGS and LDFLAGS.
Only LDAC_SOFT_FLOAT should be needed, and I'm wondering if that flag should depend on the arch ? Have you tried 

%install
rm -rf %{buildroot}
-> No need to clean the buildroot, this is done automatically.

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
-> This is unneeded for F28+. Replace with %ldconfig_scriptlets, which will expand properly depending on the target distro and release.
https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets

-> Why is this package not appropriate for Fedora ? At first glance, I don't see anything that would prevent it from being accepted and it builds fine on a a pure fedora buildroot.
Comment 4 Xavier Bachelot 2019-01-30 11:06:49 CET
Sorry for the duplicate comments, but as I had this already written, I thought it was worth posting anyway.
Comment 5 Gergely Gombos 2019-01-30 17:11:22 CET
Thanks a lot for your help!
I'm going to submit this to the main Fedora repos - #5155 will have to wait for this one.

- using %cmake solved the debuginfo problems, now there is a proper debuginfo package generated, and most of the parameters are not needed except -DLDAC_SOFT_FLOAT=OFF and -DINSTALL_LIBDIR=%{_libdir} (otherwise files will be put into /usr/lib). Super!

- I use %ldconfig_scriptlets now

- I corrected all the other suggestions.

This is an ApacheV2 licensed software module from the AOSP repos, but if one wants to build LDAC-capable hardware, it has to be licensed from Sony. So it's indeed not trivial, the Fedora Legal team will decide.

These are just for reference so that you can see what I did:

Rpmlint:
SRPM
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
RPM
libldac.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Spec:
https://pagure.io/libldac/raw/master/f/libldac.spec
SRPM:
https://pagure.io/libldac/raw/master/f/libldac-2.0.2.2-2.fc30.src.rpm
Comment 6 Gergely Gombos 2019-01-30 17:43:27 CET
Fedora submission:
https://bugzilla.redhat.com/show_bug.cgi?id=1671064
Comment 7 Gergely Gombos 2019-02-14 19:45:47 CET
Approved in Fedora, closing this bug as fixed.