Bug 1638824 - Review Request: kdsoap - Qt-based SOAP library
Summary: Review Request: kdsoap - Qt-based SOAP library
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-10-12 14:09 UTC by Casper Meijn
Modified: 2021-06-26 12:48 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-06-26 12:48:13 UTC
Type: ---
Embargoed:
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Casper Meijn 2018-10-12 14:09:53 UTC
Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00808717-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00808717-kdsoap/kdsoap-1.7.0-1.fc28.src.rpm
Description: A Qt-based client-side and server-side SOAP component 
Fedora Account System Username: caspermeijn
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=30190312

This is my first package for Fedora and therefore I need a sponsor. I use this library for an application I am developing and have contributed a few times to kdsoap.

Comment 1 Luis Segundo 2018-10-12 16:17:54 UTC
you need to make some changes

Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc-c++
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
- If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s)
  for the package is included in %license.
  Note: License file LICENSE.GPL.txt is not marked as %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Comment 2 Casper Meijn 2018-10-12 17:51:20 UTC
Thanks for the quick reaction. I cleaned up the BuildRequires and Requires and marked the license files. However I am not sure whether I used the %license macro correctly nor how to validate it.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00808791-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00808791-kdsoap/kdsoap-1.7.0-1.fc28.src.rpm

Comment 3 Luis Segundo 2018-10-12 19:29:23 UTC
Hi Casper, 

For documentation use the macro: %doc 
  and this one "%{_datarootdir}" can be removed


for License replace 
%license %{_datarootdir}/doc/KDSoap/LICENSE.txt
%license %{_datarootdir}/doc/KDSoap/LICENSE.GPL.txt
%license %{_datarootdir}/doc/KDSoap/LICENSE.US.txt  

to %doc doc/KDSoap/LICENSE*

Comment 4 Antonio T. (sagitter) 2018-10-12 20:03:48 UTC
(In reply to Luis Segundo from comment #1)
> you need to make some changes
> 
> Issues:
> =======
> - All build dependencies are listed in BuildRequires, except for any that
>   are listed in the exceptions section of Packaging Guidelines.
>   Note: These BR are not needed: gcc-c++
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

It's a false positive. 'gcc-c++' must be listed as BuildRequires package else compiler is not installed.

(In reply to Luis Segundo from comment #3)
> Hi Casper, 
> 
> For documentation use the macro: %doc 
>   and this one "%{_datarootdir}" can be removed
> 
> 
> for License replace 
> %license %{_datarootdir}/doc/KDSoap/LICENSE.txt
> %license %{_datarootdir}/doc/KDSoap/LICENSE.GPL.txt
> %license %{_datarootdir}/doc/KDSoap/LICENSE.US.txt  
> 
> to %doc doc/KDSoap/LICENSE*

'doc/KDSoap/' is the wrong place for installing license files. Casper needs to remove all LICENSE* files inside 'doc/KDSoap/', mark them with the '%license' macro and comment for indicate which files are licensed with a specific license.

See https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

Comment 5 Casper Meijn 2018-10-13 10:16:13 UTC
Am I correct that the %doc and %license marcos are for installing files from the source to the correct buildroot directory?

I now remove the license files and use the %license macro. I added a comment for the multiple licenses. And added gcc-c++ to the BuildRequires again.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00808948-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00808948-kdsoap/kdsoap-1.7.0-1.fc28.src.rpm

Comment 6 Antonio T. (sagitter) 2018-10-13 10:26:48 UTC
(In reply to Casper Meijn from comment #5)
> Am I correct that the %doc and %license marcos are for installing files from
> the source to the correct buildroot directory?
> 
> I now remove the license files and use the %license macro. I added a comment
> for the multiple licenses. And added gcc-c++ to the BuildRequires again.
> 

Yes. But you're not using '%doc' yet.
See https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

In your package:

%doc = %_pkgdocdir = %{_docdir}/%{name} != %{_datarootdir}/doc/KDSoap

Luis, please go on with your revision.

Comment 7 Casper Meijn 2018-10-15 18:29:44 UTC
(In reply to Antonio Trande from comment #6)
> %doc = %_pkgdocdir = %{_docdir}/%{name} != %{_datarootdir}/doc/KDSoap

I understand now; the doc files were installed to the wrong path. I now delete the doc files and install them using the %doc macro.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00809343-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00809343-kdsoap/kdsoap-1.7.0-1.fc28.src.rpm

Comment 8 Luis Segundo 2018-10-15 19:59:41 UTC
Nice Casper,

It is necessary to add this license "LICENSE.txt"
and  replace %{_datarootdir} to %{_datadir} on
%{_datarootdir}/mkspecs/features/kdsoap.prf

Comment 9 Casper Meijn 2018-10-17 08:14:58 UTC
Thanks Luis,

The "LICENSE.txt" is not the license under which we want to use the software. Is it then still necessary to include it?
I replaced %{_datarootdir} by %{_datadir}.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-rawhide-x86_64/00810756-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-rawhide-x86_64/00810756-kdsoap/kdsoap-1.7.0-1.fc30.src.rpm

Comment 10 Luis Segundo 2018-10-17 17:14:23 UTC
(In reply to Casper Meijn from comment #9)

> The "LICENSE.txt" is not the license under which we want to use the
> software. Is it then still necessary to include it?
I understand, if the license does not apply, it can be removed.

> I replaced %{_datarootdir} by %{_datadir}.
Nice!

please check this last issue

Issues:
=======
- If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s)
  for the package is included in %license.
  Note: License file license.cpp is not marked as %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Comment 11 Casper Meijn 2018-10-18 18:46:30 UTC
(In reply to Luis Segundo from comment #10)
> I understand, if the license does not apply, it can be removed.
I removed LICENSE.txt again

> please check this last issue
> 
> Issues:
> =======
> - If (and only if) the source package includes the text of the license(s)
>   in its own file, then that file, containing the text of the license(s)
>   for the package is included in %license.
>   Note: License file license.cpp is not marked as %license
>   See:
>   http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

I am not sure what you mean with this. I package LICENSE.GPL.txt and LICENSE.AGPL3-modified.txt, which are the two license under which we want use the code. The other license files are for a commercial license or LGPL.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00811928-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00811928-kdsoap/kdsoap-1.7.0-1.fc28.src.rpm

Comment 12 Robert-André Mauchin 🐧 2018-10-19 19:23:30 UTC
 - Not needed:

rm -rf $RPM_BUILD_ROOT

 - Not needed anymore:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

   If you plan on packaging for F27 or EPEL7, use %ldconfig_scriptlets instead.

 - To avoid unintentional soname bump, it is now forbidden to glob the major soname vension, be more specific instead:

%{_libdir}/*.so.1*

 - you seem to have forgotten to include the doc:

%files
%doc doc

 - Why? These are not doc:

%doc kdsoap.pri kdwsdl2cpp.pri

 - Why are the binaries in the devel package?

%{_bindir}/*

 - Be more specific here:

%{_includedir}/*
%{_libdir}/cmake/

Comment 13 Casper Meijn 2018-10-20 15:35:28 UTC
(In reply to Robert-André Mauchin from comment #12)
>  - Not needed:
> 
> rm -rf $RPM_BUILD_ROOT

I removed this line

> 
>  - Not needed anymore:
> 
> %post -p /sbin/ldconfig
> 
> %postun -p /sbin/ldconfig

I removed these lines

> 
>    If you plan on packaging for F27 or EPEL7, use %ldconfig_scriptlets
> instead.
> 
>  - To avoid unintentional soname bump, it is now forbidden to glob the major
> soname vension, be more specific instead:
> 
> %{_libdir}/*.so.1*

I added the library name and major version number

> 
>  - you seem to have forgotten to include the doc:
> 
> %files
> %doc doc

I added doc directory to the package, thanks

> 
>  - Why? These are not doc:
> 
> %doc kdsoap.pri kdwsdl2cpp.pri

I primarily added these because upstream does it this way. I haven't used these myself, but I believe this is needed for including the library in some applications. What would be an appropriate path for these files?

> 
>  - Why are the binaries in the devel package?
> 
> %{_bindir}/*

The kdwsdl2cpp binary is a converter for translating WSDL files to c++ code, which can be included in an application and this code uses the kdsoap library. The binary is only useful during development.

> 
>  - Be more specific here:
> 
> %{_includedir}/*
> %{_libdir}/cmake/

I added the directory names to these.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-rawhide-x86_64/00813018-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-rawhide-x86_64/00813018-kdsoap/kdsoap-1.7.0-1.fc30.src.rpm

Comment 14 Robert-André Mauchin 🐧 2018-10-20 16:22:01 UTC
 - Split tte doc into a noarch -doc subpackage

 - Own theses dirs:

/usr/share/mkspecs/features, /usr/share/mkspecs

 - Fix the line encodings:

kdsoap.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/kdsoap/doc/config/doxygen.css


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 2344960 bytes in 293 files.
  See:
  http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

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: "GNU Lesser General Public License (v2.1)", "GNU Lesser General
     Public License", "Unknown or generated", "GNU General Public License
     (v2)", "AGPL", "LGPL (v2 or v3)", "GPL (v2)", "Expat License", "GNU
     Lesser General Public License (v2 or later)", "LGPL (v2.1 or v3)",
     "*No copyright* BSD 3-clause "New" or "Revised" License". 634 files
     have unknown license. Detailed output of licensecheck in
     /home/bob/packaging/review/kdsoap/review-kdsoap/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/mkspecs/features,
     /usr/share/mkspecs
[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.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[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 does not own files or directories owned by other packages.
[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 kdsoap-
     debuginfo , kdsoap-debugsource
[?]: 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.
[-]: 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]: 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:
[!]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 2508800 bytes in /usr/share
[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]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: kdsoap-1.7.0-1.fc30.x86_64.rpm
          kdsoap-devel-1.7.0-1.fc30.x86_64.rpm
          kdsoap-debuginfo-1.7.0-1.fc30.x86_64.rpm
          kdsoap-debugsource-1.7.0-1.fc30.x86_64.rpm
          kdsoap-1.7.0-1.fc30.src.rpm
kdsoap.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/kdsoap/doc/config/doxygen.css
kdsoap-devel.x86_64: W: no-manual-page-for-binary kdwsdl2cpp
5 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 15 Robert-André Mauchin 🐧 2018-10-20 18:33:22 UTC
If you have trouble rebuilding your SRPM:

rpmbuild --define "python3_pkgversion 34" --define "_sourcedir $PWD" --define "_srcrpmdir $PWD" --target epel-7-x86_64 -bs boost-python3.spec

Comment 16 Robert-André Mauchin 🐧 2018-10-20 18:33:44 UTC
Wrong tab sorry.

Comment 17 Casper Meijn 2018-10-29 18:20:48 UTC
Thanks for your comments

(In reply to Robert-André Mauchin from comment #14)
>  - Split tte doc into a noarch -doc subpackage
> 

I created a separate doc package for the documentation

>  - Own theses dirs:
> 
> /usr/share/mkspecs/features, /usr/share/mkspecs

I don't know how to fix this. Could you give me instructions or point to the documentation? Thanks.

> 
>  - Fix the line encodings:
> 
> kdsoap.x86_64: W: wrong-file-end-of-line-encoding
> /usr/share/doc/kdsoap/doc/config/doxygen.css

I converted these using dos2unix.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00816655-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-28-x86_64/00816655-kdsoap/kdsoap-1.7.0-1.fc28.src.rpm

Comment 18 Robert-André Mauchin 🐧 2018-10-30 08:41:05 UTC
 - Own theses dirs:

/usr/share/mkspecs/features, /usr/share/mkspecs

   Use:

%dir %{datadir}/mkspecs
%dir %{datadir}/mkspecs/features

Comment 19 Casper Meijn 2018-10-30 18:54:51 UTC
(In reply to Robert-André Mauchin from comment #18)
>  - Own theses dirs:
> 
> /usr/share/mkspecs/features, /usr/share/mkspecs
> 
>    Use:
> 
> %dir %{datadir}/mkspecs
> %dir %{datadir}/mkspecs/features

Done

Spec URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-rawhide-x86_64/00817063-kdsoap/kdsoap.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/caspermeijn/kdsoap/fedora-rawhide-x86_64/00817063-kdsoap/kdsoap-1.7.0-1.fc30.src.rpm

Comment 20 Robert-André Mauchin 🐧 2018-10-30 19:26:09 UTC
The package is approved. Now you need to find a sponsor.

Comment 21 Luis Bazan 2018-10-30 19:54:39 UTC
Congratulations! Welcome aboard!

Sponsored!

FAS: caspermeijn

Regards!

Comment 22 Luis Segundo 2018-11-02 16:44:50 UTC
This was already approved and sponsored, any idea why you have not created the repository yet?

Comment 23 Casper Meijn 2018-11-02 19:02:06 UTC
Thank you all for reviewing and sponsoring me!


(In reply to Luis Segundo from comment #22)
> This was already approved and sponsored, any idea why you have not created
> the repository yet?

I just now requested the repo, I just had to find some time :-)

Comment 24 Gwyn Ciesla 2018-11-02 19:18:49 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/kdsoap

Comment 25 Mattia Verga 2021-06-26 12:48:13 UTC
Package is in repo


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