Bug 1199296 (laszip)

This is unofficial package review -

After fixing below issue, try running rpmlint on SRPM, Spec and RPMS generated from source tar to check if any error still exists.

Issues found
-------------
* Any significance of having ._configure.ac, ._Makefile.am and ._README file in source tar?
* License should be GPLv2+ instead of GLPL (correct License is LGPL) according to COPYING file
* Specifying BuildRoot is redundant and hence not needed to specify except for if building for EPEL5
 https://fedoraproject.org/wiki/How_to_create_an_RPM_package#SPEC_file_overview
* In %install section cleaning buildroot is not required
  rm -rf %{buildroot}
* %build
%configure --includedir=%{_includedir}/laszip
Why does --includedir=%{_includedir}/laszip is used to include include headers of same package here?

* Instead of make DESTDIR=%{buildroot} install, use %make_install macro
 https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25install_section
* explicit %clean for buildroot is not needed for fedora, only required if building for EPEL
 https://fedoraproject.org/wiki/How_to_create_an_RPM_package#SPEC_file_overview
* %defattr(-, root, root) is not needed from rpm 4.4 its not needed 
 https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_Permissions
* ldconfig called in %post and %postun is required.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries
* COPYING file available in source tar should be installed in /usr/share/licenses which is done by using %licence macro
%licence COPYING

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

* NEWS and README files are of zero size length, not needed to add them in %doc
* 
%{_includedir}/laszip/*.hpp
%{_libdir}/liblaszip.a
%{_libdir}/liblaszip.so*

Shared library, static library and header files, all together are present in main package.
http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages
http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

* Changelog version doesn't match with package version
* Tue Jan 13 2015 Devrim GUNDUZ <devrim> 1.3.0-1
 it should be
 * Tue Jan 13 2015 Devrim GUNDUZ <devrim> 2.2.0-1
 
 http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs


fedora-review tool output-

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

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


Issues:
======

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

C/C++:
[x]: Package does not contain any libtool archives (.la)

Generic:
[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]: 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]: Macros in Summary, %description expandable at SRPM build time.
[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 do not use a name that already exist
[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:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[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: laszip-2.2.0-1.fc21.x86_64.rpm
          laszip-2.2.0-1.fc21.src.rpm
laszip.x86_64: W: spelling-error Summary(en_US) compant -> company, compact, com pant
laszip.x86_64: W: spelling-error %description -l en_US rapidlasso -> rapid lasso, rapid-lasso, rapidness
laszip.x86_64: W: incoherent-version-in-changelog 1.3.0-1 ['2.2.0-1.fc21', '2.2.0-1']
laszip.x86_64: W: invalid-license GLPL
laszip.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/laszippertest ['/usr/lib64']
laszip.x86_64: W: devel-file-in-non-devel-package /usr/include/laszip/laszipper.hpp
laszip.x86_64: E: zero-length /usr/share/doc/laszip/NEWS
laszip.x86_64: E: library-without-ldconfig-postin /usr/lib64/liblaszip.so.6.0.0
laszip.x86_64: E: library-without-ldconfig-postun /usr/lib64/liblaszip.so.6.0.0
laszip.x86_64: W: devel-file-in-non-devel-package /usr/include/laszip/laszipexport.hpp
laszip.x86_64: W: devel-file-in-non-devel-package /usr/include/laszip/laszip.hpp
laszip.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liblaszip.so
laszip.x86_64: E: zero-length /usr/share/doc/laszip/README
laszip.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liblaszip.a
laszip.x86_64: W: devel-file-in-non-devel-package /usr/include/laszip/lasunzipper.hpp
laszip.x86_64: W: no-manual-page-for-binary laszippertest
laszip.x86_64: W: install-file-in-docs /usr/share/doc/laszip/INSTALL
laszip.src: W: spelling-error Summary(en_US) compant -> company, compact, com pant
laszip.src: W: spelling-error %description -l en_US rapidlasso -> rapid lasso, rapid-lasso, rapidness
laszip.src: W: invalid-license GLPL
laszip.src: W: invalid-url Source0: https://github.com/LASzip/LASzip/releases/download/v2.2.0/laszip-src-2.2.0.tar.gz HTTP Error 403: Forbidden
2 packages and 0 specfiles checked; 5 errors, 16 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Requires
--------
laszip (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    liblaszip.so.6()(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    rtld(GNU_HASH)



Provides
--------
laszip:
    laszip
    laszip(x86-64)
    liblaszip.so.6()(64bit)



Unversioned so-files
--------------------
laszip: /usr/lib64/liblaszip.so

Source checksums
----------------
https://github.com/LASzip/LASzip/releases/download/v2.2.0/laszip-src-2.2.0.tar.gz :
  CHECKSUM(SHA256) this package     : d0f6fa9c486caa6905927ebf32240aa7ef34181bbcc039cf8e51aa923557dc79
  CHECKSUM(SHA256) upstream package : d0f6fa9c486caa6905927ebf32240aa7ef34181bbcc039cf8e51aa923557dc79


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/bin/fedora-review -n laszip
Buildroot used: fedora-21-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

Hi,

Thanks for the review. I fixed most of the stuff, except:

(In reply to Sinny Kumari from comment #1)
> 
> Issues found
> -------------
> * Any significance of having ._configure.ac, ._Makefile.am and ._README file
> in source tar?

That comes from upstream. I will ask them to remove them in next releases.

> Why does --includedir=%{_includedir}/laszip is used to include include
> headers of same package here?

The packages that look for laszip-devel dependency are looking into /usr/include/laszip, and the Makefile installs these under /usr/include, so I had to force it to that directory.

So:

Spec URL: http://www.gunduz.org/epel/laszip.spec
SRPM URL: http://www.gunduz.org/epel/laszip-2.2.0-3.rhel7.src.rpm


Regards, Devrim

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=9497308


$ rpmlint laszip laszip-devel
laszip.i686: W: spelling-error Summary(en_US) compant -> company, compact, com pant
laszip.i686: W: spelling-error %description -l en_US rapidlasso -> rapid lasso, rapid-lasso, rapidness
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 vtable for __cxxabiv1::__class_type_info
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 vtable for __cxxabiv1::__si_class_type_info
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __cxa_pure_virtual
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 typeinfo for int
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __gxx_personality_v0
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_ostream<char, std::char_traits<char> >::seekp(std::fpos<__mbstate_t>)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_ostream<char, std::char_traits<char> >::tellp()
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 operator delete(void*)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __cxa_end_catch
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __cxa_allocate_exception
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_istream<char, std::char_traits<char> >::seekg(long long, std::_Ios_Seekdir)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_istream<char, std::char_traits<char> >::tellg()
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_istream<char, std::char_traits<char> >::get()
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 operator new(unsigned int)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_ostream<char, std::char_traits<char> >::put(char)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_istream<char, std::char_traits<char> >::seekg(std::fpos<__mbstate_t>)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 operator delete[](void*)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 operator new[](unsigned int)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __cxa_begin_catch
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __cxa_rethrow
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 __cxa_throw
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_istream<char, std::char_traits<char> >::read(char*, int)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_ostream<char, std::char_traits<char> >::write(char const*, int)
laszip.i686: W: undefined-non-weak-symbol /usr/lib/liblaszip.so.6.0.0 std::basic_ostream<char, std::char_traits<char> >::seekp(long long, std::_Ios_Seekdir)
laszip.i686: E: incorrect-fsf-address /usr/share/licenses/laszip/COPYING
laszip.i686: W: devel-file-in-non-devel-package /usr/lib/liblaszip.so
laszip.i686: W: no-manual-page-for-binary laszippertest
laszip.i686: W: install-file-in-docs /usr/share/doc/laszip/INSTALL
laszip-devel.i686: W: no-documentation
2 packages and 0 specfiles checked; 1 errors, 30 warnings.

per the warnings, several of these need to be addressed:

1.  SHOULD fix liblaszip undefined symbols, probably just missing -lstdc++ linkage, I can help come up with a patch for this (not treating as review blocker, but will need to be addressed if you expect to build anything using this).

2.  MUST move %{_libdir}/liblaszip.so symlink to -devel subpkg

3.  SHOULD omit INSTALL from %doc

naming: ok

sources: ok
1693724d8284dc04f04eb0b86f7de2cc  laszip-src-2.2.0.tar.gz


4. license: NOT ok
source files and COPYING refer to LGPLv2, so MUST use 
License: LGPLv2

5.  MUST fix %{_includedir}/laszip/ dir being unowned, I'd suggest replacing
%{_includedir}/laszip/*.hpp
with
%{_includedir}/laszip/

6.  SHOULD omit liblaszip.a static library, I'd suggest using configure flag:
%configure --disable-static

7.  -devel subpkg MUST depend on main pkg (using %_isa macro)
Requires: %{name}%{?_isa} = %{version}-%{release}

8.  SHOULD omit deprecated Group: tags and %clean section

9.  SHOULD drop not needed dependency:
BuildRequires: cmake

macros: ok

scriptlets: ok

10. MUST fix %build section missing 'make' cmd, I'd suggest adding:
make %{?_smp_mflags}

Hi Rex,

Thanks for the review. I think I fixed most of these, except item #2:

"2.  MUST move %{_libdir}/liblaszip.so symlink to -devel subpkg"

How can I do that?

Current versions are here:

Spec URL: http://www.gunduz.org/epel/laszip.spec
SRPM URL: http://www.gunduz.org/epel/laszip-2.2.0-4.f21.src.rpm

Regards, Devrim

BTW, what is the reason of dropping cmake from BR?

for 2, use instead:

%files
%{_libdir}/liblaszip.so.6*
...
%files devel
%{_libdir}/liblaszip.so


As for no cmake, because it isn't needed or used here.  This library is autoconf/automake-based.

Hi,

Thanks for the tip!

Here you go:

Spec URL: http://www.gunduz.org/epel/laszip.spec
SRPM URL: http://www.gunduz.org/epel/laszip-2.2.0-4.f21.src.rpm

Regards, Devrim

Looks good, APPROVED

Thank you!

New Package SCM Request
=======================
Package Name: laszip
Short Description: Quickly turns bulky LAS files into compant LAZ files
Upstream URL: http://www.laszip.org/
Owners: devrim
Branches: epel7 el6 f21 f22 
InitialCC: devrim

Git done (by process-git-requests).

laszip-2.2.0-4.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/laszip-2.2.0-4.fc21

laszip-2.2.0-4.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/laszip-2.2.0-4.fc22

laszip-2.2.0-4.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/laszip-2.2.0-4.el7

laszip-2.2.0-4.fc22 has been pushed to the Fedora 22 stable repository.

laszip-2.2.0-4.fc21 has been pushed to the Fedora 21 stable repository.

laszip-2.2.0-4.el7 has been pushed to the Fedora EPEL 7 stable repository.