Bug 1838686

FYI, I have also built it successfully on

COPR: https://copr.fedorainfracloud.org/coprs/neteler/pdal/builds/

koji scratch: https://koji.fedoraproject.org/koji/taskinfo?taskID=44773174

- BuildRequires:	boost
  BuildRequires:	boost-devel
=> boost-devel already pulls in boost

- BuildRequires:	bash-completion
=> Looks suspicious, why is this needed?

- BuildRequires:	proj
=> Shouldn't that be proj-devel?

- BuildRequires:	glibc-headers
=> Probably unnecessary

- Requires:	eigen3
=> eigen3 is a pure development package, shipping only headers. Why is it required?

- Requires:	gdal
  Requires:	geos
  Requires:	laszip
  Requires:	libgeotiff
  Requires:	libzstd
  Requires:	libxml2
  Requires:	postgresql
  Requires:	python3
  %if 0%{?rhel} >= 7
  Requires:	libqhull
  %else
  Requires:	qhull
  %endif
  Requires:	zlib
  Requires:	libzstd
=> These are incorrect, they should be for PDAL-libs, but the requires are automatically generated, so you can just remove all these requires (i.e. see rpm -qp --requires PDAL-libs-2.1.0-3.fc33.x86_64.rpm)

- Group:		Development/Libraries
=> Not needed anymore

- %setup -q -n %{name}-%{version}-src
=> I'd recommend to switch to %autosetup -p1 -n %{name}-%{version}-src

- 	-D CMAKE_BUILD_TYPE=Release \
=> You'll probably want -D CMAKE_BUILD_TYPE=RelWithDebInfo, although %cmake explicitly sets all CC/CXX/LDFLAGS, so you can also just drop CMAKE_BUILD_TYPE

- make %{?_smp_mflags}
=> %make_build

- make install/fast DESTDIR=%{buildroot}
=> %make_install

- %{__rm} -f %{buildroot}%{_usr}/lib/pdal/cmake/PDAL*.cmake
=> rm -f %{buildroot}%{_prefix}/lib/pdal/cmake/PDAL*.cmake (the __ prefixed macros are internal ones and should not be used)

- %postun -p /sbin/ldconfig
  %post -p /sbin/ldconfig
=> Not needed anymore post F28 (https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets)

- Files:
=> %{_bindir}/pdal-config belongs to pdal-devel
=> %{_libdir}/libpdal_base.so and %{_libdir}/libpdal_util.so belong to pdal-devel
=> Don't use wildcards for the libraries, otherwise it's easy to miss soname bumps
=> %{_libdir}/libpdal_plugin* look like pdal plugins, which are not meant to be linked against by third party applications? If so, filter them from the provides (https://docs.fedoraproject.org/en-US/packaging-guidelines/AutoProvidesAndRequiresFiltering/)
=> %{_libdir}/cmake/PDAL/PDAL*.cmake: %{_libdir}/cmake/PDAL/ is unowned, I'd just write %{_libdir}/cmake/PDAL/ (it marks the directory and all items below as owned by the package)
=> Documentation is large (63MB), you must add a dedicated PDAL-doc noarch subpackage
=> License must always be installed, move %license LICENSE.txt to PDAL-libs which all other subpackages require. For the new PDAL-doc subpackage, you can add the license a second time for that package to avoid the PDAL-doc -> PDAL-libs dependency.

- 3rd party libraries under PDAL-2.1.0-src/vendor/
=> Explicitly remove them in %prep to make sure they are not used if you can use the system copy, otherwise add a bundled(...) provides
=> If you use the bundled libraries, you'll need to add the corresponding licenses to License and %license as well (there is various non-BSD code)

- Tests: there appears to be a test suite, consider adding %check and running the test suite, or add a comment why this is not feasible

- Relevant rpmlint output:
PDAL.x86_64: E: explicit-lib-dependency libgeotiff
PDAL.x86_64: E: explicit-lib-dependency libxml2
PDAL.x86_64: E: explicit-lib-dependency libzstd
PDAL.x86_64: E: explicit-lib-dependency zlib
PDAL.x86_64: W: devel-file-in-non-devel-package /usr/bin/pdal-config
PDAL.x86_64: E: version-control-internal-file /usr/share/doc/PDAL/doc/.gitignore
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/_static/logo/Bauhaus93.ttf
PDAL.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/PDAL/doc/_static/logo/sticker/front.ai
PDAL.x86_64: W: file-not-utf8 /usr/share/doc/PDAL/doc/_static/logo/sticker/front.ai
PDAL.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/PDAL/doc/_static/logo/sticker/iheartpdal.ai
PDAL.x86_64: W: file-not-utf8 /usr/share/doc/PDAL/doc/_static/logo/sticker/iheartpdal.ai
PDAL.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/PDAL/doc/_static/logo/sticker/sticker.ai
PDAL.x86_64: W: file-not-utf8 /usr/share/doc/PDAL/doc/_static/logo/sticker/sticker.ai
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-bold-webfont.eot
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-bold-webfont.svg
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-bold-webfont.ttf
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-bold-webfont.woff
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-bold-webfont.woff2
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-regular-webfont.eot
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-regular-webfont.svg
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-regular-webfont.ttf
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-regular-webfont.woff
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/miriamlibre-regular-webfont.woff2
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-bold-webfont.eot
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-bold-webfont.svg
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-bold-webfont.ttf
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-bold-webfont.woff
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-bold-webfont.woff2
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-regular-webfont.eot
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-regular-webfont.svg
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-regular-webfont.ttf
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-regular-webfont.woff
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/pdal_rtd/static/fonts/sintony-regular-webfont.woff2
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/stages/filters.statisticaloutlier.img1.png
PDAL.x86_64: W: spurious-executable-perm /usr/share/doc/PDAL/doc/stages/filters.statisticaloutlier.img2.png
PDAL.x86_64: E: version-control-internal-file /usr/share/doc/PDAL/doc/workshop/.gitignore
PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11 exit.5
PDAL-libs.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libpdal_base.so
PDAL-libs.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libpdal_util.so
6 packages and 0 specfiles checked; 6 errors, 40 warnings.

Thanks so much for your detailed review.

I have addressed most of it but struggle with a few points (since I am not much of an expert here):

> => %{_libdir}/libpdal_plugin* look like pdal plugins, which are not meant to be linked against by third party applications?
>    If so, filter them from the provides (https://docs.fedoraproject.org/en-US/packaging-guidelines/AutoProvidesAndRequiresFiltering/)

My attempt to write a regex fails (SPEC file snippet):

# We don't want to provide private PDAL extension libs (to be verified)
# TODO: fix regex
%global __provides_exclude_from ^%{libdir}/%{libpdal_plugin}*/.*\.so$

... this currently leads to:

warning: Ignoring invalid regex ^%{libdir}/%{libpdal_plugin}*/.*.so$


Similarly, I am stuck here (SPEC file snippet):

# We don't want to include 3rd party software with unclear licenses
# TODO: fix regex
%exclude %dir %{name}-%{version}-src/vendor/

... code is not excluded.


Finally, here I don't know what it means, nor, how to fix that:

rpmlint PDAL-libs-2.1.0-3.fc33.x86_64.rpm
PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11 exit.5
PDAL-libs.x86_64: W: no-documentation
PDAL-libs.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libpdal_base.so
PDAL-libs.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libpdal_util.so


Pointers welcome!


Concerning the other DOC rpmlint warnings, I have opened an upstream ticket: https://github.com/PDAL/PDAL/issues/3090

My latest efforts are here:
https://koji.fedoraproject.org/koji/taskinfo?taskID=44832614

Created attachment 1691203 [details]
PDAL.spec

Attached a spec with some fixes:

- Correctly unbundle eigen3 and gtest
- Correctly build and run tests
- Actually build docs instead of packaging the sources
- Fix provides_excludes_from
- Move unversioned so libraries to -devel

Open issues:

- The library versioning adopted by PDAL is pretty unusual, i.e. in CMakeLists.txt

set(PDAL_API_VERSION "10")
set(PDAL_BUILD_VERSION "11")

[...]

set_target_properties(${PDAL_BASE_LIB_NAME} PROPERTIES
    VERSION ${PDAL_BUILD_VERSION}
    SOVERSION ${PDAL_API_VERSION}
    CLEAN_DIRECT_OUTPUT 1)

You'd rather expect something like

set(PDAL_API_MAJ_VERSION "10")
set(PDAL_API_MIN_VERSION "X")

[...]

set_target_properties(${PDAL_BASE_LIB_NAME} PROPERTIES
    VERSION ${PDAL_API_MAJ_VERSION}.${PDAL_API_MIN_VERSION}.0
    SOVERSION ${PDAL_API_MAJ_VERSION}
    CLEAN_DIRECT_OUTPUT 1)

Don't think this is actually a blocker for the review, but might be interesting to understand what upstream intends to achieve by using this versioning. I suspect that they are building on Windows and may not be familiar with SO versioning.


- PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11 exit.5

You should report this upstream, a shared library should never call exit, as this will result in an application using the library to quit if the respective condition is met (rather, the library should notify the application about this condition, say via exceptions or some error handler mechanism, so that the application can then choose how to proceed).

(In reply to Sandro Mani from comment #4)
> Created attachment 1691203 [details]
> PDAL.spec
> 
> Attached a spec with some fixes:
> 
> - Correctly unbundle eigen3 and gtest
> - Correctly build and run tests

Thanks for this.

> - Actually build docs instead of packaging the sources

Right, of course the way to go.
BTW, the wrong file permissions have been addressed today: https://github.com/PDAL/PDAL/pull/3093

> - Fix provides_excludes_from
> - Move unversioned so libraries to -devel

Thanks also here.

> Open issues:
> 
> - The library versioning adopted by PDAL is pretty unusual, i.e. in
> CMakeLists.txt
> 
> set(PDAL_API_VERSION "10")
> set(PDAL_BUILD_VERSION "11")
> 
> [...]


This had been fixed upstream and will be part of the next PDAL release:
https://github.com/PDAL/PDAL/pull/3042


> - PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11
> exit.5
> 
> You should report this upstream, a shared library should never call exit, as
> this will result in an application using the library to quit if the
> respective condition is met (rather, the library should notify the
> application about this condition, say via exceptions or some error handler
> mechanism, so that the application can then choose how to proceed).

I have now reported that at https://github.com/PDAL/PDAL/issues/3094

I see that you have developed two patches
- Patch0:         PDAL_unbundle.patch
- Patch1:         PDAL_tests.patch

Would you mind to add them here as well?

Created attachment 1691394 [details]
PDAL_tests.patch

Created attachment 1691395 [details]
PDAL_unbundle.patch

Yes, sure, had forgotten to do so

> This had been fixed upstream and will be part of the next PDAL release:
> https://github.com/PDAL/PDAL/pull/3042

Wonder if it makes sense to package the latest git snapshot of the 2.1-maintenance branch awaiting 2.1.1, to avoid having soname issues when you next update the package.

Concerning the soname issue, I have asked in the PDAL ticket about the timeline of the next release including the fix.


Next I have redone a scratch built, which fails in 2 tests:

98% tests passed, 2 tests failed out of 108
Total Test time (real) =  55.64 sec
The following tests FAILED:
	  1 - pgpointcloudtest (Failed)
	 85 - pdal_filters_shell_test (Failed)
Errors while running CTest
error: Bad exit status from /var/tmp/rpm-tmp.fwuQlf (%check)


In detail:


1:  PDAL_DRIVER_PATH=/builddir/build/BUILD/PDAL-2.1.0-src/lib64
1: Test timeout computed to be: 10000000
1: [==========] Running 6 tests from 1 test suite.
1: [----------] Global test environment set-up.
1: [----------] 6 tests from PgpointcloudWriterTest
1: [ RUN      ] PgpointcloudWriterTest.write
1: unknown file: Failure
1: C++ exception with description "could not connect to server: No such file or directory   <<<---- maybe impossible to test against PG?
1: 	Is the server running locally and accepting
1: 	connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?
1: " thrown in SetUp().
1: [  FAILED  ] PgpointcloudWriterTest.write (1 ms)
1: [ RUN      ] PgpointcloudWriterTest.writeScaled
1: unknown file: Failure
1: C++ exception with description "could not connect to server: No such file or directory
1: 	Is the server running locally and accepting
1: 	connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?
1: " thrown in SetUp().
1: [  FAILED  ] PgpointcloudWriterTest.writeScaled (0 ms)
1: [ RUN      ] PgpointcloudWriterTest.writeXYZ
1: unknown file: Failure
1: C++ exception with description "could not connect to server: No such file or directory
1: 	Is the server running locally and accepting
1: 	connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?
1: " thrown in SetUp().
1: [  FAILED  ] PgpointcloudWriterTest.writeXYZ (1 ms)
1: [ RUN      ] PgpointcloudWriterTest.writetNoPointcloudExtension
1: unknown file: Failure
1: C++ exception with description "could not connect to server: No such file or directory
1: 	Is the server running locally and accepting
1: 	connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?
1: " thrown in SetUp().
1: [  FAILED  ] PgpointcloudWriterTest.writetNoPointcloudExtension (0 ms)
1: [ RUN      ] PgpointcloudWriterTest.writeDeleteTable
1: unknown file: Failure
1: C++ exception with description "could not connect to server: No such file or directory
1: 	Is the server running locally and accepting
1: 	connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?
1: " thrown in SetUp().
1: [  FAILED  ] PgpointcloudWriterTest.writeDeleteTable (0 ms)
1: [ RUN      ] PgpointcloudWriterTest.selectExistingSchema
1: unknown file: Failure
1: C++ exception with description "could not connect to server: No such file or directory
1: 	Is the server running locally and accepting
1: 	connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?
1: " thrown in SetUp().
1: [  FAILED  ] PgpointcloudWriterTest.selectExistingSchema (0 ms)
1: [----------] 6 tests from PgpointcloudWriterTest (2 ms total)
1: 
1: [----------] Global test environment tear-down
1: [==========] 6 tests from 1 test suite ran. (2 ms total)
1: [  PASSED  ] 0 tests.
1: [  FAILED  ] 6 tests, listed below:
1: [  FAILED  ] PgpointcloudWriterTest.write
1: [  FAILED  ] PgpointcloudWriterTest.writeScaled
1: [  FAILED  ] PgpointcloudWriterTest.writeXYZ
1: [  FAILED  ] PgpointcloudWriterTest.writetNoPointcloudExtension
1: [  FAILED  ] PgpointcloudWriterTest.writeDeleteTable
1: [  FAILED  ] PgpointcloudWriterTest.selectExistingSchema
1: 
1:  6 FAILED TESTS
  1/108 Test   #1: pgpointcloudtest .......................***Failed    0.16 sec



85: Test command: /builddir/build/BUILD/PDAL-2.1.0-src/bin/pdal_filters_shell_test
85: Environment variables: 
85:  PDAL_DRIVER_PATH=/builddir/build/BUILD/PDAL-2.1.0-src/lib64
85: Test timeout computed to be: 10000000
85: [==========] Running 1 test from 1 test suite.
85: [----------] Global test environment set-up.
85: [----------] 1 test from ShellFilterTest
85: [ RUN      ] ShellFilterTest.test_shell_filter
85: sh: gdalinfo: command not found                                                 <<<---- gdal bins appear to be a test requirement
85: unknown file: Failure
85: C++ exception with description "Command 'gdalinfo -json /builddir/build/BUILD/PDAL-2.1.0-src/test/data/gdal/int16.tif' failed to execute with output ''" thrown in the test body.
85: [  FAILED  ] ShellFilterTest.test_shell_filter (43 ms)
85: [----------] 1 test from ShellFilterTest (43 ms total)
85: 
85: [----------] Global test environment tear-down
85: [==========] 1 test from 1 test suite ran. (43 ms total)
85: [  PASSED  ] 0 tests.
85: [  FAILED  ] 1 test, listed below:
85: [  FAILED  ] ShellFilterTest.test_shell_filter
85: 
85:  1 FAILED TEST


Not sure how to deal with esp. the first one. The second might be solved with another BuildRequires dependency.

Here the link to the scratch built with the respective root.log: https://koji.fedoraproject.org/koji/taskinfo?taskID=44897107

Adding BR: /usr/bin/gdalinfo will solve the second one, the first one might be solved by adding BR: postgresql-server.

I have modified the BRs as follows:

--- /home/mneteler/rpmbuild/SPECS/PDAL.spec	2020-05-24 19:16:26.794094506 +0200
+++ PDAL.spec	2020-05-24 18:50:52.810157029 +0200
@@ -32,6 +32,7 @@
 BuildRequires:	cmake
 BuildRequires:	eigen3-devel
 BuildRequires:	gcc-c++
+BuildRequires:	gdal
 BuildRequires:	gdal-devel
 BuildRequires:	geos-devel
 BuildRequires:	gtest-devel
@@ -39,10 +40,12 @@
 BuildRequires:	jsoncpp-devel
 BuildRequires:	laszip-devel
 BuildRequires:	libgeotiff-devel
+BuildRequires:	libpq-devel
 BuildRequires:	libxml2-devel
 BuildRequires:	libzstd-devel
 BuildRequires:	netcdf-cxx-devel
 BuildRequires:	postgresql-devel
+BuildRequires:	postgresql-server
 BuildRequires:	proj-devel
 BuildRequires:	python3-breathe
 BuildRequires:	python3-devel

Still the PG related test fails:

https://koji.fedoraproject.org/koji/taskinfo?taskID=44909226

Created attachment 1691596 [details]
updated PDAL spec file

Created attachment 1691769 [details]
updated PDAL spec file

Successfully compiling

Problem of `pgpointcloudtest` addressed, it now compile successfully: https://koji.fedoraproject.org/koji/taskinfo?taskID=44944553

Is there any other test failing other than pgpointcloudtest? It would be better to leave out the " || echo "Ignoring test failures"", as otherwise no-one will notice the test failures.

Once you believe that all is addressed, please post a new SPEC+SRPM to finish the review.

(In reply to Sandro Mani from comment #17)
> Is there any other test failing other than pgpointcloudtest?

Yes, only this one. FWIW, in the openSuSe PDAL package they doesn't test at all and in Debian pgpointcloudtest is excluded (I got that from there).


> It would be
> better to leave out the " || echo "Ignoring test failures"", as otherwise
> no-one will notice the test failures.

OK, but I don't get it running.

> OK, but I don't get it running.

How so? Are there other errors?

Concerning the library versioning (answer by the PDAL maintainer):

> The library versioning adopted by PDAL is pretty unusual, i.e. in CMakeLists.txt
This was a bug that is fixed in https://github.com/PDAL/PDAL/pull/3042. This is merged to the 2.1-maintenance branch and will be included in PDAL 2.1.1, which is expected by the end of June 2020.


Concerning the failing pgpointcloudtest:

- https://github.com/PDAL/PDAL/issues/742 "Test for pgpointcloud fails"

First I tried to define the PG related env vars:

%check
# test the compiled code (see doc/project/testing.rst)
PGUSER=pdal PGPASSWORD=password PGHOST=localhost PGPORT=5432 ctest -V

Also now this test is still failing ("could not connect to server: Connection refused").
I suspect that during the RPM compilation the PG server is simply not running and I have no idea how to start that in the SPEC file.
Debian seems to face the same test problem, they have work-arounded it like this:

ctest -V --exclude-regex "pgpointcloudtest"

Now, 
- we can deactivate the PostgreSQL support entirely as I was not able to find so far a SPEC file with working ctest-running PostgreSQL combo.
- or skip the pgpointcloudtest test.

Hence, I now adopted the next suggestion from above PDAL ticket 742 to skip the test:
-D BUILD_PGPOINTCLOUD_TESTS:BOOL=OFF



Latest log:
https://koji.fedoraproject.org/koji/taskinfo?taskID=44957096

successes:
- i686
- x86_64


remaining failures:
- armv7hl  (https://kojipkgs.fedoraproject.org//work/tasks/7211/44957211/build.log):
 Start  38: pdal_io_las_writer_test
38: Test command: /builddir/build/BUILD/PDAL-2.1.0-src/bin/pdal_io_las_writer_test
38: Environment variables: 
38:  PDAL_DRIVER_PATH=/builddir/build/BUILD/PDAL-2.1.0-src/lib
38: Test timeout computed to be: 10000000
38: [==========] Running 27 tests from 1 test suite.
38: [----------] Global test environment set-up.
38: [----------] 27 tests from LasWriterTest
38: [ RUN      ] LasWriterTest.srs
38: [       OK ] LasWriterTest.srs (35 ms)
38: [ RUN      ] LasWriterTest.srs2
38: [       OK ] LasWriterTest.srs2 (12 ms)
38: [ RUN      ] LasWriterTest.auto_offset
38: [       OK ] LasWriterTest.auto_offset (10 ms)
38: [ RUN      ] LasWriterTest.auto_offset2
38: [       OK ] LasWriterTest.auto_offset2 (15 ms)
38: [ RUN      ] LasWriterTest.extra_dims
38: [       OK ] LasWriterTest.extra_dims (16 ms)
38: [ RUN      ] LasWriterTest.all_extra_dims
38: [       OK ] LasWriterTest.all_extra_dims (24 ms)
38: [ RUN      ] LasWriterTest.forward
38: (readers.las Error) GDAL failure (1) PROJ: proj_create_from_database: crs not found
38: (readers.las Error) GDAL failure (1) PROJ: proj_create_from_database: crs not found
38: [       OK ] LasWriterTest.forward (1204 ms)
38: [ RUN      ] LasWriterTest.forwardvlr
38: proj_uom_get_info_from_database: unit of measure not found
38: [       OK ] LasWriterTest.forwardvlr (85 ms)
38: [ RUN      ] LasWriterTest.flex
38: [       OK ] LasWriterTest.flex (16 ms)
38: [ RUN      ] LasWriterTest.flex2
38: [       OK ] LasWriterTest.flex2 (11 ms)
38: [ RUN      ] LasWriterTest.laszip
38: [       OK ] LasWriterTest.laszip (797 ms)
38: [ RUN      ] LasWriterTest.laszip1_4
 38/107 Test  #38: pdal_io_las_writer_test ................Bus error***Exception:   3.11 sec


aarch64 (https://kojipkgs.fedoraproject.org//work/tasks/7214/44957214/build.log):
        Start  89: pdal_filters_stats_test
89: Test command: /builddir/build/BUILD/PDAL-2.1.0-src/bin/pdal_filters_stats_test
89: Environment variables: 
89:  PDAL_DRIVER_PATH=/builddir/build/BUILD/PDAL-2.1.0-src/lib64
89: Test timeout computed to be: 10000000
89: [==========] Running 9 tests from 1 test suite.
89: [----------] Global test environment set-up.
89: [----------] 9 tests from Stats
89: [ RUN      ] Stats.handcalc
89: [       OK ] Stats.handcalc (2 ms)
89: [ RUN      ] Stats.baseline
89: [       OK ] Stats.baseline (2 ms)
89: [ RUN      ] Stats.simple
89: [       OK ] Stats.simple (2 ms)
89: [ RUN      ] Stats.advanced
89: /builddir/build/BUILD/PDAL-2.1.0-src/test/unit/filters/StatsFilterTest.cpp:211: Failure
89: The difference between statsX.sampleSkewness() and -5.2235397e-16 is 1.9919314947420312e-18, which exceeds 1e-23, where
89: statsX.sampleSkewness() evaluates to -5.2036203850525794e-16,
89: -5.2235397e-16 evaluates to -5.2235396999999997e-16, and
89: 1e-23 evaluates to 9.9999999999999996e-24.
89: /builddir/build/BUILD/PDAL-2.1.0-src/test/unit/filters/StatsFilterTest.cpp:212: Failure
89: The difference between statsY.sampleSkewness() and -5.7098153e-16 is 1.1898719537472993e-20, which exceeds 1e-23, where
89: statsY.sampleSkewness() evaluates to -5.7096963128046251e-16,
89: -5.7098153e-16 evaluates to -5.7098152999999998e-16, and
89: 1e-23 evaluates to 9.9999999999999996e-24.
89: /builddir/build/BUILD/PDAL-2.1.0-src/test/unit/filters/StatsFilterTest.cpp:213: Failure
89: The difference between statsZ.sampleSkewness() and -5.5176534e-16 is 6.8715912016394351e-19, which exceeds 1e-23, where
89: statsZ.sampleSkewness() evaluates to -5.5245249912016398e-16,
89: -5.5176534e-16 evaluates to -5.5176534000000004e-16, and
89: 1e-23 evaluates to 9.9999999999999996e-24.
89: [  FAILED  ] Stats.advanced (2 ms)
89: [ RUN      ] Stats.stream
89: [       OK ] Stats.stream (1 ms)
89: [ RUN      ] Stats.dimset
89: [       OK ] Stats.dimset (2 ms)
89: [ RUN      ] Stats.metadata
89: [       OK ] Stats.metadata (1 ms)
89: [ RUN      ] Stats.enum
89: [       OK ] Stats.enum (1 ms)
89: [ RUN      ] Stats.global
89: [       OK ] Stats.global (1 ms)
89: [----------] 9 tests from Stats (14 ms total)
89: 
89: [----------] Global test environment tear-down
89: [==========] 9 tests from 1 test suite ran. (14 ms total)
89: [  PASSED  ] 8 tests.
89: [  FAILED  ] 1 test, listed below:
89: [  FAILED  ] Stats.advanced
89: 
89:  1 FAILED TEST
 89/107 Test  #89: pdal_filters_stats_test ................***Failed    0.19 sec


- s390x (https://kojipkgs.fedoraproject.org//work/tasks/7216/44957216/build.log):
The following tests FAILED:
	  9 - pdal_metadata_test (Failed)
	 19 - pdal_polygon_test (Failed)
	 21 - pdal_spatial_reference_test (Failed)
	 32 - pdal_io_ept_reader_test (Failed)
	 37 - pdal_io_las_reader_test (Failed)
	 38 - pdal_io_las_writer_test (Failed)
	 45 - pdal_io_qfit_test (Failed)
	 48 - pdal_io_terrasolid_test (Failed)
	 56 - pdal_filters_crop_test (Failed)
	 77 - pdal_filters_overlay_test (Failed)
	 79 - pdal_filters_reprojection_test (Failed)
	 89 - pdal_filters_stats_test (Failed)
	 91 - pdal_filters_hexbin_test (Failed)
	 96 - pdal_info_test (Failed)
	 97 - pdal_tile_test (Failed)


In addition I tried EPEL8:
https://koji.fedoraproject.org/koji/taskinfo?taskID=44959014

DEBUG util.py:600:  No matching package to install: 'gdal'
DEBUG util.py:600:  No matching package to install: 'gdal-devel'
DEBUG util.py:600:  No matching package to install: 'laszip-devel'
DEBUG util.py:600:  No matching package to install: 'python3-breathe'
DEBUG util.py:600:  No matching package to install: 'python3-sphinxcontrib-bibtex'
DEBUG util.py:600:  Not all dependencies satisfied

This might be addressed at a later point (locally I have already compiled PDAL on EPEL8 without sphinx docs).


My question is: do all architecture have to be supported to get PDAL accepted in Fedora?

Created attachment 1691970 [details]
updated PDAL spec file

As for the test, you can ignore the test failures on problematic arches, say

%ifarch armv7hl aarch64 s390x
ctest || true
%else
ctest
%endif

Generally it's not mandatory for tests to pass, but especially for the main arches it's good pratice to not silently ignore failures if the test suite can be run in such way that it passes 100%, it can be helpful to expose subtile packaging issues later on, say if a dependency changes somehow.

As suggested, I have modified the test to ignore the test failures on problematic arches.

neteler's scratch build of PDAL-2.1.0-4.fc31.src.rpm for f33 completed
	http://koji.fedoraproject.org/koji/taskinfo?taskID=45000793

Build successfully:

    rebuildSRPM (noarch)
    buildArch (PDAL-2.1.0-4.fc33.src.rpm, armv7hl)
    buildArch (PDAL-2.1.0-4.fc33.src.rpm, i686)
    buildArch (PDAL-2.1.0-4.fc33.src.rpm, x86_64)
    buildArch (PDAL-2.1.0-4.fc33.src.rpm, aarch64)
    buildArch (PDAL-2.1.0-4.fc33.src.rpm, ppc64le)
    buildArch (PDAL-2.1.0-4.fc33.src.rpm, s390x)

Created attachment 1692236 [details]
PDAL spec file

Posting links for fedora review:

Spec URL: https://smani.fedorapeople.org/review/PDAL.spec
SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/806/45000806/PDAL-2.1.0-4.fc33.src.rpm

(In reply to markusN from comment #5)
> (In reply to Sandro Mani from comment #4)
> > - PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11
> > exit.5
> > 
> > You should report this upstream, a shared library should never call exit, as
> > this will result in an application using the library to quit if the
> > respective condition is met (rather, the library should notify the
> > application about this condition, say via exceptions or some error handler
> > mechanism, so that the application can then choose how to proceed).
> 
> I have now reported that at https://github.com/PDAL/PDAL/issues/3094

FYI - also this one now fixed upstream:
"Remove all the exit() calls in the poisson filter stuff" (https://github.com/PDAL/PDAL/pull/3098)

Created attachment 1692463 [details]
licensecheck.txt

Full review below. Issues:

[!]: License field in the package spec file matches the actual license.
=> In bundled code and elsewhere, in addition to BSD you also have code licensed ASL2.0, Expat and various flavours of the boost licence. You'll need to add the licenses of code which is actually compiled into the libraries (so i.e. not tests) to the License field and add a comment on the license breakdown. See attached licensecheck.txt for details.

PDAL.src:157: E: hardcoded-library-path in %{_prefix}/lib/pdal/cmake/PDAL*.cmake
=> Can't spot where rpmlint picked this one out, I'd quickly check if the installed cmake files actually work (since the path referenced there does not exist), and if yes, then I'd ignore this one

Rest looks good.




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

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


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "BSD 3-clause "New" or "Revised"
     License", "*No copyright* BSD 3-clause "New" or "Revised" License",
     "BSD 3-clause "New" or "Revised" License Apache License 2.0", "Expat
     License", "*No copyright* Apache License 2.0", "Boost Software License
     1.0", "BSD 2-clause "Simplified" License", "Apache License 2.0", "*No
     copyright* Boost Software License 1.0", "Boost Software License 1.0
     [generated file]", "NTP License", "SIL Open Font License 1.1",
     "zlib/libpng license Boost Software License 1.0", "Public domain Boost
     Software License 1.0", "NTP License Boost Software License 1.0". 1253
     files have unknown license. Detailed output of licensecheck in
     /home/sandro/Desktop/1838686-PDAL/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Avoid bundling fonts in non-fonts packages.
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[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.
[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.


Installation errors
-------------------
INFO: mock.py version 2.3 starting (python version = 3.8.3)...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
INFO: Signal handler active
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled package manager cache
Start: cleaning package manager metadata
Finish: cleaning package manager metadata
INFO: enabled HW Info plugin
Mock Version: 2.3
INFO: Mock Version: 2.3
Finish: chroot init
INFO: installing package(s): /home/sandro/Desktop/1838686-PDAL/results/PDAL-debuginfo-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-libs-debuginfo-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-debugsource-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-devel-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-doc-2.1.0-4.fc33.noarch.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-libs-2.1.0-4.fc33.x86_64.rpm
ERROR: Command failed: 
 # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 29 --setopt=deltarpm=False --allowerasing --disableplugin=local --disableplugin=spacewalk install /home/sandro/Desktop/1838686-PDAL/results/PDAL-debuginfo-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-libs-debuginfo-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-debugsource-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-devel-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-2.1.0-4.fc33.x86_64.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-doc-2.1.0-4.fc33.noarch.rpm /home/sandro/Desktop/1838686-PDAL/results/PDAL-libs-2.1.0-4.fc33.x86_64.rpm --setopt=tsflags=nocontexts



Rpmlint
-------
Checking: PDAL-2.1.0-4.fc33.x86_64.rpm
          PDAL-devel-2.1.0-4.fc33.x86_64.rpm
          PDAL-libs-2.1.0-4.fc33.x86_64.rpm
          PDAL-doc-2.1.0-4.fc33.noarch.rpm
          PDAL-debuginfo-2.1.0-4.fc33.x86_64.rpm
          PDAL-debugsource-2.1.0-4.fc33.x86_64.rpm
          PDAL-2.1.0-4.fc33.src.rpm
PDAL.x86_64: W: no-documentation
PDAL.x86_64: W: no-manual-page-for-binary pdal
PDAL-devel.x86_64: W: no-documentation
PDAL-devel.x86_64: W: no-manual-page-for-binary pdal-config
PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11 exit.5
PDAL-libs.x86_64: W: no-documentation
PDAL-doc.noarch: W: hidden-file-or-dir /usr/share/doc/PDAL-doc/html/.buildinfo
PDAL-doc.noarch: W: hidden-file-or-dir /usr/share/doc/PDAL-doc/html/.doctrees
PDAL-doc.noarch: W: hidden-file-or-dir /usr/share/doc/PDAL-doc/html/.doctrees
PDAL-doc.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/PDAL-doc/html/_static/logo/sticker/front.ai
PDAL-doc.noarch: W: file-not-utf8 /usr/share/doc/PDAL-doc/html/_static/logo/sticker/front.ai
PDAL-doc.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/PDAL-doc/html/_static/logo/sticker/iheartpdal.ai
PDAL-doc.noarch: W: file-not-utf8 /usr/share/doc/PDAL-doc/html/_static/logo/sticker/iheartpdal.ai
PDAL-doc.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/PDAL-doc/html/_static/logo/sticker/sticker.ai
PDAL-doc.noarch: W: file-not-utf8 /usr/share/doc/PDAL-doc/html/_static/logo/sticker/sticker.ai
PDAL.src:63: W: unversioned-explicit-provides bundled(arbiter)
PDAL.src:65: W: unversioned-explicit-provides bundled(PoissonRecon)
PDAL.src:67: W: unversioned-explicit-provides bundled(nanoflann)
PDAL.src:69: W: unversioned-explicit-provides bundled(nlohmann)
PDAL.src:157: E: hardcoded-library-path in %{_prefix}/lib/pdal/cmake/PDAL*.cmake
PDAL.src:160: W: macro-in-comment %{buildroot}
PDAL.src:160: W: macro-in-comment %{_prefix}
PDAL.src:161: W: macro-in-comment %{buildroot}
PDAL.src:161: W: macro-in-comment %{_prefix}
PDAL.src:162: W: macro-in-comment %{buildroot}
PDAL.src:162: W: macro-in-comment %{_prefix}
PDAL.src:110: W: mixed-use-of-spaces-and-tabs (spaces: line 110, tab: line 1)
7 packages and 0 specfiles checked; 1 errors, 26 warnings.




Source checksums
----------------
https://github.com/PDAL/PDAL/releases/download/2.1.0/PDAL-2.1.0-src.tar.gz :
  CHECKSUM(SHA256) this package     : c300de7935d52cb96e24bdaceea5d189b1840e88636e6deca1f6dad51f909571
  CHECKSUM(SHA256) upstream package : c300de7935d52cb96e24bdaceea5d189b1840e88636e6deca1f6dad51f909571


Requires
--------
PDAL (rpmlib, GLIBC filtered):
    PDAL-libs(x86-64)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libpdal_base.so.10()(64bit)
    libpdal_util.so.10()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    rtld(GNU_HASH)

PDAL-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    /usr/bin/sh
    PDAL-libs(x86-64)
    cmake-filesystem(x86-64)
    libpdal_base.so.10()(64bit)
    libpdal_plugin_kernel_fauxplugin.so.10()(64bit)
    libpdal_plugin_reader_pgpointcloud.so.10()(64bit)
    libpdal_plugin_writer_pgpointcloud.so.10()(64bit)
    libpdal_util.so.10()(64bit)
    pkgconfig(gdal)
    pkgconfig(libxml-2.0)

PDAL-libs (rpmlib, GLIBC filtered):
    ld-linux-x86-64.so.2()(64bit)
    libc.so.6()(64bit)
    libcurl.so.4()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgdal.so.27()(64bit)
    libgeotiff.so.5()(64bit)
    liblaszip.so.8()(64bit)
    libm.so.6()(64bit)
    libpdal_base.so.10()(64bit)
    libpdal_util.so.10()(64bit)
    libpq.so.5()(64bit)
    libpq.so.5(RHPG_9.6)(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.1)(64bit)
    libstdc++.so.6(CXXABI_1.3.2)(64bit)
    libstdc++.so.6(CXXABI_1.3.5)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libxml2.so.2()(64bit)
    libxml2.so.2(LIBXML2_2.4.30)(64bit)
    libxml2.so.2(LIBXML2_2.5.8)(64bit)
    libxml2.so.2(LIBXML2_2.6.0)(64bit)
    libxml2.so.2(LIBXML2_2.6.2)(64bit)
    libxml2.so.2(LIBXML2_2.6.23)(64bit)
    libxml2.so.2(LIBXML2_2.6.5)(64bit)
    libz.so.1()(64bit)
    libzstd.so.1()(64bit)
    rtld(GNU_HASH)

PDAL-doc (rpmlib, GLIBC filtered):

PDAL-debuginfo (rpmlib, GLIBC filtered):

PDAL-debugsource (rpmlib, GLIBC filtered):



Provides
--------
PDAL:
    PDAL
    PDAL(x86-64)
    bundled(PoissonRecon)
    bundled(arbiter)
    bundled(nanoflann)
    bundled(nlohmann)

PDAL-devel:
    PDAL-devel
    PDAL-devel(x86-64)
    cmake(PDAL)
    cmake(pdal)
    pkgconfig(pdal)

PDAL-libs:
    PDAL-libs
    PDAL-libs(x86-64)
    libpdal_base.so.10()(64bit)
    libpdal_util.so.10()(64bit)

PDAL-doc:
    PDAL-doc

PDAL-debuginfo:
    PDAL-debuginfo
    PDAL-debuginfo(x86-64)
    debuginfo(build-id)

PDAL-debugsource:
    PDAL-debugsource
    PDAL-debugsource(x86-64)



Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16
Command line :/usr/bin/fedora-review -b 1838686
Buildroot used: fedora-rawhide-x86_64
Active plugins: C/C++, Generic, Shell-api
Disabled plugins: Haskell, R, Python, PHP, SugarActivity, Java, Perl, Ocaml, fonts
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

(In reply to Sandro Mani from comment #27)
> Created attachment 1692463 [details]
> licensecheck.txt
> 
> Full review below.

Thanks for the new review.

> Issues:
> 
> [!]: License field in the package spec file matches the actual license.
> => In bundled code and elsewhere, in addition to BSD you also have code
> licensed ASL2.0, Expat and various flavours of the boost licence. You'll
> need to add the licenses of code which is actually compiled into the
> libraries (so i.e. not tests) to the License field and add a comment on the
> license breakdown. See attached licensecheck.txt for details.

I went through the licensecheck.txt file and verified the unclear extractions within the files.
Would the following change be ok (twice in the spec file)? 

-%license LICENSE.txt
+%license Apache License 2.0 and Boost Software License 1.0 and BSD 2-clause "Simplified" License and BSD 3-clause "New" or "Revised" License and Expat License and NTP License and SIL Open Font License 1.1 and zlib/libpng license

> PDAL.src:157: E: hardcoded-library-path in
> %{_prefix}/lib/pdal/cmake/PDAL*.cmake
> => Can't spot where rpmlint picked this one out, I'd quickly check if the
> installed cmake files actually work (since the path referenced there does
> not exist), and if yes, then I'd ignore this one

It is here:

   132	# Remove duplicated cmake files
   133	rm -f %{buildroot}%{_prefix}/lib/pdal/cmake/PDAL*.cmake

I can comment it out and see what happens (not being familiar with cmake).


> Rest looks good.

Great news!

> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> ===== MUST items =====

[...]

> Generic:
[...]
> [!]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses
>      found: "Unknown or generated", "BSD 3-clause "New" or "Revised"
>      License", "*No copyright* BSD 3-clause "New" or "Revised" License",
>      "BSD 3-clause "New" or "Revised" License Apache License 2.0", "Expat
>      License", "*No copyright* Apache License 2.0", "Boost Software License
>      1.0", "BSD 2-clause "Simplified" License", "Apache License 2.0", "*No
>      copyright* Boost Software License 1.0", "Boost Software License 1.0
>      [generated file]", "NTP License", "SIL Open Font License 1.1",
>      "zlib/libpng license Boost Software License 1.0", "Public domain Boost
>      Software License 1.0", "NTP License Boost Software License 1.0". 1253
>      files have unknown license. Detailed output of licensecheck in
>      /home/sandro/Desktop/1838686-PDAL/licensecheck.txt

...see above.

[...]

> Installation errors
> -------------------

[...]

> /home/sandro/Desktop/1838686-PDAL/results/PDAL-libs-2.1.0-4.fc33.x86_64.rpm
> ERROR: Command failed: 
>  # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/
> --releasever 29 --setopt=deltarpm=False --allowerasing --disableplugin=local
> --disableplugin=spacewalk install

This is strange: no error message provided.


[...]

> Rpmlint
> -------

[...]

> PDAL-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpdal_base.so.11
> exit.5

... already fixed upstream (will be part of the July 2020 release).

[...]
> /usr/share/doc/PDAL-doc/html/_static/logo/sticker/front.ai
> PDAL-doc.noarch: W: file-not-utf8
> /usr/share/doc/PDAL-doc/html/_static/logo/sticker/front.ai
> PDAL-doc.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/PDAL-doc/html/_static/logo/sticker/iheartpdal.ai
> PDAL-doc.noarch: W: file-not-utf8
> /usr/share/doc/PDAL-doc/html/_static/logo/sticker/iheartpdal.ai
> PDAL-doc.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/PDAL-doc/html/_static/logo/sticker/sticker.ai
> PDAL-doc.noarch: W: file-not-utf8
> /usr/share/doc/PDAL-doc/html/_static/logo/sticker/sticker.ai

... already fixed upstream (will be part of the July 2020 release).

[...]
> PDAL.src:157: E: hardcoded-library-path in
> %{_prefix}/lib/pdal/cmake/PDAL*.cmake

...see above.

> I went through the licensecheck.txt file and verified the unclear extractions within the files.
> Would the following change be ok (twice in the spec file)? 

> -%license LICENSE.txt
> +%license Apache License 2.0 and Boost Software License 1.0 and BSD 2-clause "Simplified" License and BSD 3-clause "New" or "Revised" License and Expat License and NTP License and SIL Open Font License 1.1 > and zlib/libpng license

No, it's the License field in the head section where you need to list all the license names, and then include the respective license files via %license in the corresponding package.

Actually, looking closer at the boost stuff, it appears that pdalboost is just a bundled boost, and of all that code only boost_filesystem is actually used in pdal/util/FileUtils.cpp. It's easy to unbundle, see [1], but you might want to ask upstream why they felt the need to bundle and renamespace the boost as pdalboost just for that one module. In particular, they might just want to use std::filesystem and do away with boost altogether.

So, this said, as far as the license is concerned you need to use the license names listed here [2], so you'd write something like

# The code is licensed BSD except for:
# - filters/private/csf/* and plugins/i3s/lepcc/* are ASL 2.0
# - vendor/arbiter/*, plugins/nitf/io/nitflib.h and plugins/oci/io/OciWrapper.* are Expat/MIT
# - plugins/e57/libE57Format/{src,include}/* is Boost
License: BSD and ASL 2.0 and MIT and Boost

License files to include in the libs package:
PDAL-2.1.0-src/LICENSE.txt
PDAL-2.1.0-src/vendor/arbiter/LICENSE
PDAL-2.1.0-src/plugins/e57/libE57Format/LICENSE.md

where I've omitted everything under pdalboost as it's now unbundled.

[1] https://smani.fedorapeople.org/review/PDAL/
[2] https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Software_License_List

Thanks for the updates, much appreciated!

New RPMs:
https://koji.fedoraproject.org/koji/taskinfo?taskID=45074748

rpmlint only shows a few warnings (see above, two have already been addressed upstream).

Created attachment 1692849 [details]
PDAL_unbundle.patch

Created attachment 1692850 [details]
PDAL.spec

Only remaining remark: what's with these

> # Remove duplicated cmake files (rpmlint complains)
> #rm -f %%{buildroot}%%{_prefix}/lib/pdal/cmake/PDAL*.cmake

I suppose you can just drop those lines from the spec? It's not like these file are appearing in any %files section.

Everything else looks good, approved (take care of the above lines when importing the package as you see necessary).

Created attachment 1692936 [details]
PDAL.spec

I have dropped those lines and uploaded the final (?) SPEC file again.

LGTM, feel free to proceed to request the repo & import.

Thanks for all your guidance!

https://pagure.io/releng/fedora-scm-requests/issue/25323

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/PDAL

FEDORA-2020-39bc8f5bab has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-39bc8f5bab

FEDORA-2020-39bc8f5bab has been pushed to the Fedora 32 testing repository.
In short time you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2020-39bc8f5bab \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-39bc8f5bab

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Unfortunately there is still an issue (BZ#1841616):

Your package (PDAL) Fails To Install in Fedora 33:

can't install PDAL-devel:
  - nothing provides libpdal_plugin_kernel_fauxplugin.so.10()(64bit) needed by PDAL-devel-2.1.0-5.fc33.x86_64
  - nothing provides libpdal_plugin_reader_pgpointcloud.so.10()(64bit) needed by PDAL-devel-2.1.0-5.fc33.x86_64
  - nothing provides libpdal_plugin_writer_pgpointcloud.so.10()(64bit) needed by PDAL-devel-2.1.0-5.fc33.x86_64

I am a bit surprised since rpmlint didn't complain.

Oh - just drop the provides filtering in this case, I thought they were .so only, but indeed also the versioned symlinks exist.

Just to be sure, you mean to change like this?

diff --git a/PDAL.spec b/PDAL.spec
index d3a4d94..1a41e56 100644
--- a/PDAL.spec
+++ b/PDAL.spec
@@ -200,9 +200,6 @@ ctest -V
 %files devel
 %{_bindir}/pdal-config
 %{_includedir}/pdal/
-%{_libdir}/libpdal_plugin_kernel_fauxplugin.so
-%{_libdir}/libpdal_plugin_reader_pgpointcloud.so
-%{_libdir}/libpdal_plugin_writer_pgpointcloud.so
 %{_libdir}/libpdal_base.so
 %{_libdir}/libpdal_util.so
 %{_libdir}/libpdalcpp.so

Two variants:

a) If no third-party consumer of libpdal_plugin_* exists (which I would expect), then yes, drop the unversioned symblinks
b) Otherwise, drop these two lines

# We don't want to provide private PDAL extension libs (to be verified)
%global __provides_exclude_from ^%{_libdir}/libpdal_plugin.*\.so.*$

Note that more than just 

-%{_libdir}/libpdal_plugin_kernel_fauxplugin.so
-%{_libdir}/libpdal_plugin_reader_pgpointcloud.so
-%{_libdir}/libpdal_plugin_writer_pgpointcloud.so

you'll want to %exlcude or rm them, otherwise you'll get a build failure due to unpackages files.

FEDORA-2020-35e0ac7cce has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-35e0ac7cce

FEDORA-2020-35e0ac7cce has been pushed to the Fedora 32 testing repository.
In short time you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2020-35e0ac7cce`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-35e0ac7cce

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Well, still some issues - from the new BZ#1843094:

Your package (PDAL) Fails To Install in Fedora 33:

can't install PDAL-libs:
  - nothing provides libboost_filesystem.so.1.69.0()(64bit) needed by PDAL-libs-2.1.0-6.fc33.x86_64

Again, a pointer would be welcome.

That's due to the boost 1.73 rebuild which started 2020-05-28, see [1].

[1] https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/XEHCL2HROZQXQXQUWZF26VVCPAYFEGR5/

Ah, I see. I have requested there to add PDAL to the list, it was immediately done:

https://src.fedoraproject.org/rpms/PDAL/c/0850914050c8e76ca5bd933d0bfe1a58a5d5dfb0?branch=master

FEDORA-EPEL-2020-ed997a6971 has been submitted as an update to Fedora EPEL 8. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2020-ed997a6971

FEDORA-EPEL-2020-ed997a6971 has been pushed to the Fedora EPEL 8 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2020-ed997a6971

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

PDAL-2.1.0-6.fc32 has been pushed to the Fedora 32 stable repository. If problems still persist, please make note of it in this bug report.

FEDORA-EPEL-2020-ed997a6971 has been pushed to the Fedora EPEL 8 stable repository.
If problem still persists, please make note of it in this bug report.