Bug 1199298 (liblas)

rpmls (and rpm -qlvp …) are your friends. Highly recommended reading:

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
 -> https://fedoraproject.org/wiki/Packaging:UnownedDirectories

Hi Michael,

Thank you for the comment. I improved the spec file significantly:


Spec URL: http://www.gunduz.org/epel/liblas.spec
SRPM URL: http://www.gunduz.org/epel/liblas-1.8.0-2.f21.src.rpm 

Regards, Devrim

naming: ok

1. license: NOT ok.  close though, there's some bundled modified boost headers included, so MUST use:
License: BSD and Boost
in that vein, due to how dependencies work out, I'd suggest droping
%license COPYING
(it's an empty) file, and include in -libs instead:
%license LICENSE.txt

sources: ok
599881281d45db4ce9adb2d75458391e  libLAS-1.8.0.tar.bz2

2. macros: NOT ok, MUST use %cmake macro in place of bare 'cmake', doing so, you can drop -DCMAKE_INSTALL_PREFIX:PATH=/usr flag.  while you're at it, can omit:
"../%{name}-%{version}/" too

3. MUST use %{?_isa} macro in subpkg dependencies, so main pkg and -devel should use instead:
Requries: %{name}-libs%{?_isa} = %{version}-%{release}

4. main pkg MUST drop not needed explicit dep:
Requires: laszip

5.  SHOULD omit deprecated rpm Group: tags and %clean section

6. %build section MUST envoke 'make', include:
make %{?_smp_mflags}
(after cmake call)

7.  %install section, SHOULD use
make install/fast DESTDIR=%{buildroot}
(in preference over %make_install macro, which is generally intended for autoconf-based pkgs)

8. MUST move liblas.so and liblas_c.so symlinks to -devel pkg

9. -devel MUST own
%dir %{_includedir}/liblas
%dir %{_datadir}/cmake/libLAS-%{version}
%{_datadir}/%{name}/doc/
dirs, I'd suggest removing these %dir calls and include in -devel just:
%{_includedir}/liblas/
%{_datadir}/cmake/libLAS-%{version}/
%{_datadir}/%{name}/doc/
(while we're at it, will probably have to patch this to put cmake files under %_libdir instead of %_datadir, but I can help with that later, possibly post-review)

10. SHOULD drop INSTALL from %doc

11. in %build SHOULD use
-DWITH_LASZIP:BOOL=ON
instead of
-DWITH_LASZIP:BOOL=OFF

12. in %build SHOULD replace hard-coded:
-DGEOTIFF_INCLUDE_DIR:STRING="/usr/include/libgeotiff"
with
-DGEOTIFF_INCLUDE_DIR:PATH="$(pkg-config --variable=includedir libgeotiff)"

13. MUST fix rpath.  there's some rpath's getting set somewhere, e.g.
ERROR   0001: file '/usr/bin/las2las' contains a standard rpath '/usr/lib' in [/usr/lib]
I'll try to figure out where this is coming from.

14.  SHOULD build with -DWITH_PKGCONFIG:BOOL=ON flag (unless you have some good reason not to)

regarding 13 for now, you can use cmake build flag: -DCMAKE_SKIP_RPATH:BOOL=ON as a big hammer to kill rpaths.  Long term, looks like apps/CMakeLists.txt has some silly hard-coded rpath handling that upstream needs to fix.

Hi,

I think I fixed everything:

Spec URL: http://www.gunduz.org/epel/liblas.spec
SRPM URL: http://www.gunduz.org/epel/liblas-1.8.0-3.f21.src.rpm 

One of these changes affected the build though -- we cannot install liblas-config anymore.

Regards, Devrim

I'd venture liblas-config is built only if pkgconfig is disabled (I'd argue pkgconfig is superior in that regard).

That said, 

15.  both the pkgconfig and/or liblas-config MUST be in the -devel subpkg instead.

Item 3 is not fixed correctly you added
Requires:	%{name}-libs%{?_isa} = %{version}-%{release}
to -libs subpkg (itself! :) ) instead of -devel.  it should replace -devel's:
Requires:	%{name} = %{version}-%{release}

Hi,

Ok, put liblas-config back into the package, and fixed #3:

Spec URL: http://www.gunduz.org/epel/liblas.spec
SRPM URL: http://www.gunduz.org/epel/liblas-1.8.0-3.f21.src.rpm 


Regards, Devrim

I guess we can stick with liblas-config since that is the upstream default, but I suspect it may be prone to problems like multilib conflicts (something that pkgconfig handles well).  I'd suggest you try contacting upstream  why pkgconfig isn't enabled by default

mostly cosmetic, but as part of item 9, main pkg still has:

%dir %{_includedir}/liblas
%dir %{_datadir}/cmake/libLAS-%{version}

those can be dropped now (since they are properly owned by -devel subpkg now), but I won't treat that as a blocker... you can fix post-review.


APPROVED

Thanks for the great and fast review!

New Package SCM Request
=======================
Package Name: liblas
Short Description: LAS 1.0/1.1/1.2 ASPRS LiDAR data translation toolset
Upstream URL: http://www.liblas.org/
Owners: devrim
Branches: epel7 el6 f21 f22 
InitialCC: devrim

Git done (by process-git-requests).

liblas-1.8.0-3.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/liblas-1.8.0-3.el7

liblas-1.8.0-3.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/liblas-1.8.0-3.fc22

liblas-1.8.0-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/liblas-1.8.0-3.fc21

liblas-1.8.0-3.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/liblas-1.8.0-3.el7

liblas-1.8.0-3.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/liblas-1.8.0-3.fc22

liblas-1.8.0-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/liblas-1.8.0-3.fc21

liblas-1.8.0-3.el7 has been pushed to the Fedora EPEL 7 testing repository.

liblas-1.8.0-3.fc22 has been pushed to the Fedora 22 stable repository.

liblas-1.8.0-3.fc21 has been pushed to the Fedora 21 stable repository.

liblas-1.8.0-3.el7 has been pushed to the Fedora EPEL 7 stable repository.