Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: libLAS - Library and tools for the LAS LiDAR format
https://bugzilla.redhat.com/show_bug.cgi?id=666301
Summary: Review Request: libLAS - Library and tools for the LAS LiDAR format Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: ed@eh3.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #1 from Ed Hill ed@eh3.com 2010-12-29 22:42:09 EST --- The SRPM builds in mock for Fedora-devel and Fedora-14.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
Ralf Corsepius rc040203@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rc040203@freenet.de
--- Comment #2 from Ralf Corsepius rc040203@freenet.de 2010-12-30 01:58:59 EST --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #3 from Ed Hill ed@eh3.com 2010-12-30 10:53:36 EST --- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |volker27@gmx.at
--- Comment #4 from Volker Fröhlich volker27@gmx.at 2010-12-31 19:47:37 EST --- 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_.27verb... 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #5 from Ed Hill ed@eh3.com 2011-01-12 13:18:54 EST --- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #6 from Ed Hill ed@eh3.com 2011-01-12 20:42:21 EST --- 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!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #7 from Ralf Corsepius rc040203@freenet.de 2011-01-15 12:46:01 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
Rich Mattes richmattes@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |richmattes@gmail.com
--- Comment #8 from Rich Mattes richmattes@gmail.com 2011-02-02 13:48:17 EST --- I see that libLAS 1.6.0 has been released. Are you planning to update the submission soon?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #9 from Ralf Corsepius rc040203@freenet.de 2011-04-25 04:34:39 EDT --- Ed, any update?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #10 from Volker Fröhlich volker27@gmx.at 2011-09-12 02:18:50 EDT --- Ed, are you still interested in maintaining this package?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
--- Comment #11 from Volker Fröhlich volker27@gmx.at 2011-10-08 03:15:29 EDT --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=666301
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Blocks| |201449(FE-DEADREVIEW) Resolution| |NOTABUG Status Whiteboard| |StalledSubmitter Last Closed| |2011-12-13 16:13:59
package-review@lists.fedoraproject.org