Bug 1199298 (liblas) - Review Request: liblas - LAS 1.0/1.1/1.2 ASPRS LiDAR data translation toolset
Summary: Review Request: liblas - LAS 1.0/1.1/1.2 ASPRS LiDAR data translation toolset
Keywords:
Status: CLOSED ERRATA
Alias: liblas
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: laszip
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-03-05 21:28 UTC by Devrim Gündüz
Modified: 2015-05-06 17:26 UTC (History)
3 users (show)

Fixed In Version: liblas-1.8.0-3.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-04-30 11:40:34 UTC
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Devrim Gündüz 2015-03-05 21:28:34 UTC
Spec URL: http://www.gunduz.org/epel/liblas.spec
SRPM URL: http://www.gunduz.org/epel/liblas-1.8.0-1.rhel7.src.rpm 
Description: libLAS is a C/C++ library for reading and writing the very common LAS LiDAR format. The ASPRS LAS format is a sequential binary format used to store data from LiDAR sensors and by LiDAR processing software for data interchange and archival.

Fedora Account System Username: devrim

Note: I am not sure about the license in here.liblas-1.8.0-1.rhel7.src.rpm  liblas.spec

Comment 1 Michael Schwendt 2015-03-11 11:58:09 UTC
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

Comment 2 Devrim Gündüz 2015-04-17 11:17:58 UTC
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

Comment 3 Rex Dieter 2015-04-17 12:44:22 UTC
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.

Comment 4 Rex Dieter 2015-04-17 12:49:10 UTC
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.

Comment 5 Devrim Gündüz 2015-04-17 14:52:06 UTC
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

Comment 6 Rex Dieter 2015-04-17 15:08:23 UTC
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}

Comment 7 Devrim Gündüz 2015-04-17 15:16:26 UTC
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

Comment 8 Rex Dieter 2015-04-17 15:20:25 UTC
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

Comment 9 Devrim Gündüz 2015-04-17 21:09:40 UTC
Thanks for the great and fast review!

Comment 10 Devrim Gündüz 2015-04-17 21:14:20 UTC
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

Comment 11 Gwyn Ciesla 2015-04-19 21:30:49 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2015-04-20 09:39:07 UTC
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

Comment 13 Fedora Update System 2015-04-20 09:39:16 UTC
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

Comment 14 Fedora Update System 2015-04-20 09:40:25 UTC
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

Comment 15 Fedora Update System 2015-04-20 09:43:24 UTC
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

Comment 16 Fedora Update System 2015-04-20 09:43:30 UTC
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

Comment 17 Fedora Update System 2015-04-20 09:43:36 UTC
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

Comment 18 Fedora Update System 2015-04-20 18:35:39 UTC
liblas-1.8.0-3.el7 has been pushed to the Fedora EPEL 7 testing repository.

Comment 19 Fedora Update System 2015-04-30 11:40:34 UTC
liblas-1.8.0-3.fc22 has been pushed to the Fedora 22 stable repository.

Comment 20 Fedora Update System 2015-04-30 11:49:05 UTC
liblas-1.8.0-3.fc21 has been pushed to the Fedora 21 stable repository.

Comment 21 Fedora Update System 2015-05-06 17:26:34 UTC
liblas-1.8.0-3.el7 has been pushed to the Fedora EPEL 7 stable repository.


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