Bug 666301 - Review Request: libLAS - Library and tools for the LAS LiDAR format
Summary: Review Request: libLAS - Library and tools for the LAS LiDAR format
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: StalledSubmitter
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2010-12-30 02:50 UTC by Ed Hill
Modified: 2011-12-13 21:13 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-13 21:13:59 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Ed Hill 2010-12-30 02:50:35 UTC
Spec URL: http://eh3.com/libLAS.spec
SRPM URL: http://eh3.com/libLAS-1.6.0-6.fc14.src.rpm
Description: Library and tools for the LAS LiDAR format

Comment 1 Ed Hill 2010-12-30 03:42:09 UTC
The SRPM builds in mock for Fedora-devel and Fedora-14.

Comment 2 Ralf Corsepius 2010-12-30 06:58:59 UTC
MUSTFIX:

* /usr/bin/liblas-config is not multilib ready.
This script contains hard-coded references to architecture-specific paths
=> the *-devel.i686 package and the *-devel.x86_64 package can not be installed in parallel.

One common work-around to such issues is replacing such *-config scripts by pkg-config files and reimplementing the *-config script as a wrapper around pkg-config.


* /usr/bin/liblas-config is broken:

e.g.:
a) /usr/bin/liblas-config --libs
-L/usr/lib -llas -llas_c -L/usr/lib64 optimized;/usr/lib64/libboost_program_options-mt.so;debug;/usr/lib64/libboost_program_options-mt.so /usr/lib64/libgeotiff.so /usr/lib64/libtiff.so /usr/lib64/libxml2.so

Note:
  + -L/usr/lib (on x86_64).
  + optimized;...;debug;...

b) /usr/bin/liblas-config --cflags
-I/usr/include
/usr/bin/liblas-config --includes
-I/usr/include -I/usr/include/libgeotiff -I/usr/include -I/usr/include/libxml2

Using -I/usr/include disturbs the system include path and is never right.

c) /usr/bin/liblas-config --cxxflags
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -fPIC -pedantic -ansi -Wall -Wpointer-arith -Wcast-align -Wcast-qual -Wfloat-equal -Wredundant-decls -Wno-long-long -std=c++98

Comment 3 Ed Hill 2010-12-30 15:53:36 UTC
Hi Ralf, thank you for the very quick and helpful feedback!  Here's an 
updated SRPM:

  http://eh3.com/libLAS-1.6.0-7.fc14.src.rpm

which does a better job of using the project-defined lib install 
directory (fixes the one rpmlint error) and, as an interim measure, 
removes the non-multilib-compliant /usr/bin/liblas-config from the 
RPMs.

I recently got libLAS commit permission so, pending approval by the 
project team, I'll add some CMake templates which create and install 
multilib-compliant %{_libdir}/liblas.pc files as you describe above.  
And if my bash skills aren't too pathetic I'll also write a new
liblas-config which, as you suggest, wraps pkg-config.

Would you be willing to approve the above version?

Comment 4 Volker Fröhlich 2011-01-01 00:47:37 UTC
A few things I noticed:

The verbose flag for make is not necessary, because CMAKE_VERBOSE_MAKEFILE is already set to "on", when you're using %cmake. See http://www.vtk.org/Wiki/CMake_FAQ#Is_there_an_option_to_produce_more_.27verbose.27_compiling.3F and rpm -E %cmake

Please give a rationale in the spec why you're deleting $RPMBUILD/usr/share.

The chmod seems unnecessary, since all files in $RPMBUILD/usr/bin are already 755.

I think there are some tests and Python bindings as well, is that correct?

Comment 5 Ed Hill 2011-01-12 18:18:54 UTC
Hi Ralf & Volker,

An update is available here:

  http://eh3.com/libLAS.spec
  http://eh3.com/libLAS-1.6.0-10.fc14.src.rpm

and it addresses most of the above comments.  The pkgconfig bits are 
not yet sorted out but I'll try to have an update soon.

Aside from the pkgconfig bits, do you spot any blockers?

Comment 6 Ed Hill 2011-01-13 01:42:21 UTC
Here's the promised update:

  http://eh3.com/libLAS-1.6.0-13.fc14.src.rpm

It addresses all the following:

  0) improved the pkgconfig (multi-lib) situation -- checked 
     into upstream
  1) redundant make-verbose flag removed
  2) rationale added for $RPMBUILD/usr/share
  3) description of %check section added
  4) chmod of $RPMBUILD/usr/bin removed

and there are a couple of to-do items:

  + enable -DWITH_GDAL:BOOL=ON as soon as the GDAL 1.8.x dependency
    is available and then add the necessary GDAL libs to pkgconfig
  + package the python bindings -- I will very likely need help
    with the python packaging or perhaps someone else could handle 
    it -- is anyone interested?

And thanks again for taking a look!

Comment 7 Ralf Corsepius 2011-01-15 17:46:01 UTC
Further remarks:

MUSTFIXES:

* The *.pc is broken:
...
libdir=/usr//usr/lib64
...

A detail, I am not sure about how pkgconfig will handle it, is the version number: Version: 1.6.0b4


* AFAICT, several "Requires:" are missing from *-devel.
E.g. libstdc++-devel (package uses libstdc++ headers)


* Some of the headers contain questionable defines:
# grep -R HAVE_ usr/include/*
usr/include/liblas/transform.hpp:#ifdef HAVE_GDAL
usr/include/liblas/transform.hpp:#ifdef HAVE_GDAL
usr/include/liblas/transform.hpp:#ifdef HAVE_GDAL
usr/include/liblas/detail/reader/zipreader.hpp:#ifdef HAVE_LASZIP
usr/include/liblas/detail/reader/zipreader.hpp:#endif // HAVE_LASZIP
usr/include/liblas/detail/zippoint.hpp:#ifdef HAVE_LASZIP
usr/include/liblas/detail/zippoint.hpp:#endif // HAVE_LASZIP
usr/include/liblas/detail/writer/zipwriter.hpp:#ifdef HAVE_LASZIP
usr/include/liblas/detail/writer/zipwriter.hpp:#endif // HAVE_LASZIP

- These defines collide with autoconf defines. 
A proper way to fix this to append such defines with a package specific prefix (e.g. _LIBLAS ...) and to let the package generate a header file providing these defines + to let all files using these defines include it.

- liblas need to defined/undefine these defines. Otherwise applications using this library are at risk of using a different API/ABI than the library. Though this occasionally may make sense, I doubt this is intentional.


SHOULD/CONSIDER:
* Finally, the tarball name being used is causing me headaches, but I am not familiar with mercurial and don't know off-head what the FPG says.

Comment 8 Rich Mattes 2011-02-02 18:48:17 UTC
I see that libLAS 1.6.0 has been released.  Are you planning to update the submission soon?

Comment 9 Ralf Corsepius 2011-04-25 08:34:39 UTC
Ed, any update?

Comment 10 Volker Fröhlich 2011-09-12 06:18:50 UTC
Ed, are you still interested in maintaining this package?

Comment 11 Volker Fröhlich 2011-10-08 07:15:29 UTC
I guess this review is stalled. Ed, if you are still interested, please respond!

Otherwise I'll close this bug as described in http://fedoraproject.org/wiki/Policy_for_stalled_package_reviews

Feel free to re-open it any time though.


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