Bug 1428383 - Review Request: compat-freerdp12 - Compatibility version of the FreeRDP client libraries
Summary: Review Request: compat-freerdp12 - Compatibility version of the FreeRDP clien...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Luya Tshimbalanga
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-03-02 13:15 UTC by Simone Caronni
Modified: 2017-03-22 17:29 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2017-03-06 17:44:45 UTC
Type: ---
Embargoed:
luya: fedora-review+


Attachments (Terms of Use)

Description Simone Caronni 2017-03-02 13:15:32 UTC
Spec URL: https://slaanesh.fedorapeople.org/compat-freerdp12.spec
SRPM URL: https://slaanesh.fedorapeople.org/compat-freerdp12-1.2.0-1.fc25.src.rpm
Description: Free implementation of the Remote Desktop Protocol (RDP) protocol.
This compatibility package only contains client libraries.
Fedora Account System Username: slaanesh

Comment 1 Luya Tshimbalanga 2017-03-03 07:02:56 UTC
Some suggestions:

%setup -qn FreeRDP-%{version}-beta1-android9
%patch0 -p1
%patch1 -p1
%patch2 -p1

Change it to 
%autosetup -p1 -n FreeRDP-%{version}-beta1-android9

Move LICENSE file to %license

Please fix:
compat-freerdp12.x86_64: W: shared-lib-calls-exit /usr/lib64/libfreerdp.so.1.2.0 exit.5
compat-freerdp12.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libfreerdp.so.1.2.0 SSL_CTX_set_cipher_list



Rpmlint (installed packages) generated the message below
----------------------------
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp.so.1.2.0 /lib64/libdl.so.2
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp.so.1.2.0 /lib64/libz.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp.so.1.2.0 /lib64/librt.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp.so.1.2.0 /lib64/libm.so.6
compat-freerdp12.x86_64: W: shared-lib-calls-exit /usr/lib64/libfreerdp.so.1.2.0 exit.5
compat-freerdp12.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libfreerdp.so.1.2.0 SSL_CTX_set_cipher_list
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libssl.so.10
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libcrypto.so.10
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libz.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libX11.so.6
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libxkbfile.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libjpeg.so.62
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/librt.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libm.so.6
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfreerdp-client.so.1.2.0 /lib64/libdl.so.2
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libwinpr.so.1.1.0 /lib64/libz.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libxfreerdp-client.so.1.2.0 /lib64/libdl.so.2
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libxfreerdp-client.so.1.2.0 /lib64/libxkbfile.so.1
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libxfreerdp-client.so.1.2.0 /lib64/libjpeg.so.62
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libxfreerdp-client.so.1.2.0 /lib64/libssl.so.10
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libxfreerdp-client.so.1.2.0 /lib64/libcrypto.so.10
compat-freerdp12.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libxfreerdp-client.so.1.2.0 /lib64/libz.so.1

It would be nice to address them but they are just warning messages.

Comment 2 Simone Caronni 2017-03-03 08:23:30 UTC
Spec URL: https://slaanesh.fedorapeople.org/compat-freerdp12.spec
SRPM URL: https://slaanesh.fedorapeople.org/compat-freerdp12-1.2.0-2.fc25.src.rpm

* Fri Mar 03 2017 Simone Caronni <negativo17> - 1.2.0-2
- Link libraries as needed, add license macro, use autosetup macro.

These can be ignored:

compat-freerdp12-devel.x86_64: W: spelling-error %description -l en_US libs -> lobs, lib, lbs
compat-freerdp12-devel.x86_64: W: only-non-binary-in-usr-lib
compat-freerdp12-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 4 warnings.

This would be nice to fix, but I don't know how:

compat-freerdp12.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libfreerdp.so.1.2.0 SSL_CTX_set_cipher_list

Comment 3 Luya Tshimbalanga 2017-03-03 17:50:55 UTC
1(In reply to Simone Caronni from comment #2)
> This would be nice to fix, but I don't know how:
> 
> compat-freerdp12.x86_64: W: crypto-policy-non-compliance-openssl
> /usr/lib64/libfreerdp.so.1.2.0 SSL_CTX_set_cipher_list

See https://fedoraproject.org/wiki/Packaging:CryptoPolicies. I found a similar review of that case https://bugzilla.redhat.com/show_bug.cgi?id=1198498


Also use %make_build which is the equivalent for make %{?_smp_mflags}

Once both above part are corrected, the review will be approved. Below lines are for formality.



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Apache (v2.0)", "Unknown or generated", "MIT/X11 (BSD like)",
     "*No copyright* Apache", "zlib/libpng", "ISC", "Unicode strict", "BSL
     (v1.0)", "BSD (3 clause)", "BSD (2 clause)", "MPL (v2.0)", "*No
     copyright* Apache (v2.0)". 712 files have unknown license. Detailed
     output of licensecheck in /home/luya/Documents/fedora-
     packaging/review/1428383-compat-freerdp12/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[-]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in compat-
     freerdp12-devel , compat-freerdp12-debuginfo
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: compat-freerdp12-1.2.0-2.fc25.x86_64.rpm
          compat-freerdp12-devel-1.2.0-2.fc25.x86_64.rpm
          compat-freerdp12-debuginfo-1.2.0-2.fc25.x86_64.rpm
          compat-freerdp12-1.2.0-2.fc25.src.rpm
compat-freerdp12.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libfreerdp.so.1.2.0 SSL_CTX_set_cipher_list
compat-freerdp12-devel.x86_64: W: spelling-error %description -l en_US libs -> lobs, lib, lbs
compat-freerdp12-devel.x86_64: W: only-non-binary-in-usr-lib
compat-freerdp12-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 4 warnings.




Rpmlint (debuginfo)
-------------------
Checking: compat-freerdp12-debuginfo-1.2.0-2.fc25.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
compat-freerdp12.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libfreerdp.so.1.2.0 SSL_CTX_set_cipher_list
compat-freerdp12-devel.x86_64: W: spelling-error %description -l en_US libs -> lobs, lib, lbs
compat-freerdp12-devel.x86_64: W: only-non-binary-in-usr-lib
compat-freerdp12-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 4 warnings.



Requires
--------
compat-freerdp12 (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libX11.so.6()(64bit)
    libXcursor.so.1()(64bit)
    libXext.so.6()(64bit)
    libXfixes.so.3()(64bit)
    libXi.so.6()(64bit)
    libXinerama.so.1()(64bit)
    libXrender.so.1()(64bit)
    libXv.so.1()(64bit)
    libasound.so.2()(64bit)
    libasound.so.2(ALSA_0.9)(64bit)
    libasound.so.2(ALSA_0.9.0rc4)(64bit)
    libc.so.6()(64bit)
    libcrypto.so.10()(64bit)
    libcrypto.so.10(libcrypto.so.10)(64bit)
    libcups.so.2()(64bit)
    libdl.so.2()(64bit)
    libfreerdp-client.so.1.2()(64bit)
    libfreerdp.so.1.2()(64bit)
    libglib-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgsm.so.1()(64bit)
    libgstapp-1.0.so.0()(64bit)
    libgstreamer-1.0.so.0()(64bit)
    libgstvideo-1.0.so.0()(64bit)
    libjpeg.so.62()(64bit)
    libjpeg.so.62(LIBJPEG_6.2)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libpulse.so.0()(64bit)
    libpulse.so.0(PULSE_0)(64bit)
    librt.so.1()(64bit)
    libssl.so.10()(64bit)
    libssl.so.10(libssl.so.10)(64bit)
    libwinpr.so.1.1()(64bit)
    libxkbfile.so.1()(64bit)
    rtld(GNU_HASH)

compat-freerdp12-debuginfo (rpmlib, GLIBC filtered):

compat-freerdp12-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    cmake
    compat-freerdp12(x86-64)
    libfreerdp-client.so.1.2()(64bit)
    libfreerdp.so.1.2()(64bit)
    libwinpr.so.1.1()(64bit)
    libxfreerdp-client.so.1.2()(64bit)
    pkgconfig
    pkgconfig(libssl)
    pkgconfig(winpr)
    pkgconfig(zlib)



Provides
--------
compat-freerdp12:
    compat-freerdp12
    compat-freerdp12(x86-64)
    libfreerdp-client.so.1.2()(64bit)
    libfreerdp.so.1.2()(64bit)
    libwinpr.so.1.1()(64bit)
    libxfreerdp-client.so.1.2()(64bit)

compat-freerdp12-debuginfo:
    compat-freerdp12-debuginfo
    compat-freerdp12-debuginfo(x86-64)

compat-freerdp12-devel:
    cmake(FreeRDP)
    cmake(WinPR)
    compat-freerdp12-devel
    compat-freerdp12-devel(x86-64)
    pkgconfig(freerdp)
    pkgconfig(winpr)



Unversioned so-files
--------------------
compat-freerdp12: /usr/lib64/freerdp/audin-client-alsa.so
compat-freerdp12: /usr/lib64/freerdp/audin-client-pulse.so
compat-freerdp12: /usr/lib64/freerdp/audin-client.so
compat-freerdp12: /usr/lib64/freerdp/disp-client.so
compat-freerdp12: /usr/lib64/freerdp/drive-client.so
compat-freerdp12: /usr/lib64/freerdp/echo-client.so
compat-freerdp12: /usr/lib64/freerdp/parallel-client.so
compat-freerdp12: /usr/lib64/freerdp/printer-client.so
compat-freerdp12: /usr/lib64/freerdp/rdpei-client.so
compat-freerdp12: /usr/lib64/freerdp/rdpgfx-client.so
compat-freerdp12: /usr/lib64/freerdp/rdpsnd-client-alsa.so
compat-freerdp12: /usr/lib64/freerdp/rdpsnd-client-pulse.so
compat-freerdp12: /usr/lib64/freerdp/serial-client.so
compat-freerdp12: /usr/lib64/freerdp/tsmf-client-alsa-audio.so
compat-freerdp12: /usr/lib64/freerdp/tsmf-client-gstreamer-decoder.so
compat-freerdp12: /usr/lib64/freerdp/tsmf-client-pulse-audio.so
compat-freerdp12: /usr/lib64/freerdp/tsmf-client.so

Source checksums
----------------
https://github.com/FreeRDP/FreeRDP/archive/1.2.0-beta1+android9.tar.gz :
  CHECKSUM(SHA256) this package     : b4e7810d90ee8a81075674c4aa9412dcd5409980a653f9059c05969927253ca0
  CHECKSUM(SHA256) upstream package : b4e7810d90ee8a81075674c4aa9412dcd5409980a653f9059c05969927253ca0

Comment 4 Luya Tshimbalanga 2017-03-03 17:59:20 UTC
Reading about https://fedoraproject.org/wiki/Packaging:CryptoPolicies, it looks like the system will take care of it and there is nothing else to do.

Do not forget to update other changes. That review is now APPROVED.

Comment 5 Simone Caronni 2017-03-06 13:29:33 UTC
Spec URL: https://slaanesh.fedorapeople.org/compat-freerdp12.spec
SRPM URL: https://slaanesh.fedorapeople.org/compat-freerdp12-1.2.0-2.fc25.src.rpm

I managed to fix the SSL issue above and added the %make_build macro. rpmlint still reports about the hardcoding of ciphers but is actually not happening:

-settings->PermittedTLSCiphers = arg->Value ? _strdup("ALL:!ECDH") : NULL;
+settings->PermittedTLSCiphers = arg->Value ? _strdup("PROFILE=SYSTEM") : NULL;

Thanks for the review!

Comment 6 Gwyn Ciesla 2017-03-06 15:33:09 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/compat-freerdp12

Comment 7 Luya Tshimbalanga 2017-03-07 04:04:03 UTC
(In reply to Simone Caronni from comment #5)

> -settings->PermittedTLSCiphers = arg->Value ? _strdup("ALL:!ECDH") : NULL;
> +settings->PermittedTLSCiphers = arg->Value ? _strdup("PROFILE=SYSTEM") :
> NULL;
> 
> Thanks for the review!

No problem, the guide suggested to remove the call should that line fails.

Comment 8 Dominik 'Rathann' Mierzejewski 2017-03-16 11:54:06 UTC
Sorry, but the package name doesn't follow Naming Guidelines:
https://fedoraproject.org/wiki/Packaging:Naming#Multiple_packages_with_the_same_base_name

This shouldn't have been approved.

Comment 9 Luya Tshimbalanga 2017-03-22 17:29:09 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #8)
> Sorry, but the package name doesn't follow Naming Guidelines:
> https://fedoraproject.org/wiki/Packaging:
> Naming#Multiple_packages_with_the_same_base_name
> 
> This shouldn't have been approved.

It is a old convention name to retain compatibility. I'll take a note in a future.


Note You need to log in before you can comment on or make changes to this bug.