[Bug 976793] Review Request: lunchbox - C++ library for multi-threaded programming

https://bugzilla.redhat.com/show_bug.cgi?id=976793

--- Comment #41 from Jaroslav Škarvada  ---
(In reply to Otto Urpelainen from comment #37)
> Review taken, here is everything I could spot.
> > # CMake/common/ittnotify.h is under GPLv2, the rest is LGPL
>
> I am not sure if it counts, since it is part of the build script only,
> and License field should describe the packaged content, not srpm.
> Reference:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> LicensingGuidelines/#_license_field

Upstream ticket:
https://github.com/Eyescale/Lunchbox/issues/331

The file is dual licensed, it seems upstream chose BSD and released the result
under LGPL.
> Looking at licensecheck.txt generated by fedora-review
> and disregarding the build script,
> it looks like the actual license is more like this:
> (
>   and Boost    # lunchbox/atomic.h, lunchbox/any.h
>   and LGPLv2   # most files
>   and LGPLv3   # some files, like any.cpp and lfVector.h
> )
>
> According to GPL Compatibility Matrix from Fedora Wiki [1]
> LGPLv2 and LGPLv3 mix only by "converting to GPLv3".
> I suppose the correct action would then be
> to mark the license as "Boost and GPLv3",
> note somewhere that the conversion option was used
> (I am not aware of specific instructions on how to do this)
> and include GPLv3 license text as %license.
> The Boost license is already included in ACKNOWLEDGEMENTS.txt,
> so that should be installed as %license.

Didn't you think "Boost and LGPLv3"?

I asked upstream about the status:
https://github.com/Eyescale/Lunchbox/issues/331
> Additionally, lunchbox/test.h is under BSD.
> It is a test runner,
> contains its license in the file header
> and is not compiled during the build.
> As long as it goes to devel package or is simply not installed,
> its license is ok.

IMHO the resulting work can be released under LGPL.
> > Provides:      bundled(eyescale-cmake-common)
>
> Use versioned Provides.
> Reference:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

Fixed.
> > %{_libdir}/libLunchbox.so.1*
>
> Globbing that hides important parts of the shared object file name should
> not be used.
> Because the found suffixes here are 1.17.0 and 10.0.0,
> it would be better to select both of these separately.
> Reference:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> #_listing_shared_library_files

Hopefully fixed.
> > Requires:      pkgconfig
> > Requires:      cmake
>
> I do not think these are needed.
> The build does not produce a pkgconfig file
> and CMake files are installed under self-owner directory %{_datadir}/Lunchbox

Dropped.
> I agree Petr regarding the contents of /usr/share/Lunchbox,
> those should not go to the binary package.
> > /usr/share/Lunchbox/CMake/LunchboxConfig.cmake
> > /usr/share/Lunchbox/CMake/LunchboxConfigVersion.cmake
> > /usr/share/Lunchbox/CMake/LunchboxTargets-debug.cmake
> > /usr/share/Lunchbox/CMake/LunchboxTargets.cmake
> > /usr/share/Lunchbox/CMake/options.cmake
>
> I am not familiar enough with CMake builds in Fedora
> to say how cmake scripts should be packaged
> (the CMake guidelines should be extended to cover this)
> but assuming that the directory structure is acceptable
> these should be package in the -devel package.
> > /usr/share/Lunchbox/benchmarks/perf-lfVector
> > /usr/share/Lunchbox/benchmarks/perf-mutex
> > /usr/share/Lunchbox/benchmarks/perf-rwLock
>
> Test executables, should go to -devel
> if packaged at all.
> Also, binaries have no place in %{_datadir},
> rpmlint complains about this.
> So if they are package in any package,
> they should go to %{_bindir}.
> > /usr/share/Lunchbox/tests/any.cpp
> > /usr/share/Lunchbox/tests/anySerialization.cpp
> > /usr/share/Lunchbox/tests/buffer.cpp
> > /usr/share/Lunchbox/tests/clock.cpp
> > /usr/share/Lunchbox/tests/debug.cpp
> > /usr/share/Lunchbox/tests/dso.cpp
> > /usr/share/Lunchbox/tests/file.cpp
> > /usr/share/Lunchbox/tests/future.cpp
> > /usr/share/Lunchbox/tests/init.cpp
> > /usr/share/Lunchbox/tests/issue1.cpp
> > /usr/share/Lunchbox/tests/lfQueue.cpp
> > /usr/share/Lunchbox/tests/memoryMap.cpp
> > /usr/share/Lunchbox/tests/monitor.cpp
> > /usr/share/Lunchbox/tests/mtQueue.cpp
> > /usr/share/Lunchbox/tests/perThread.cpp
> > /usr/share/Lunchbox/tests/perf/lfVector.cpp
> > /usr/share/Lunchbox/tests/perf/mutex.cpp
> > /usr/share/Lunchbox/tests/perf/rwLock.cpp
> > /usr/share/Lunchbox/tests/pluginFactory.cpp
> > /usr/share/Lunchbox/tests/refPtr.cpp
> > /usr/share/Lunchbox/tests/requestHandler.cpp
> > /usr/share/Lunchbox/tests/rng.cpp
> > /usr/share/Lunchbox/tests/thread.cpp
> > /usr/share/Lunchbox/tests/threadPool.cpp
>
> Test source files.
> I do not see any reason to package these,
> they are already available in srpm
> and it is not usual to package sources in -devel.

Hopefully fixed.
> > $ rpm -q --requires ./review-lunchbox/results/lunchbox-1.17.0-3.fc35.x86_64.rpm
> > libboost_unit_test_framework.so.1.75.0()(64bit)
>
> As this package is not a test runner or similar tool
> there should be no unit test framework dependencies.
> Probably this is solved when all the test related files
> are moved to -devel or removed from installation.

Moved to devel, hopefully OK.
> I would consider removing directory pthreads in %prep
> as it contains a shady tarball
> apprently only needed for Windows builds.
> This is not blocking,
> because as far as I can tell,
> there is nothing that is strictly disallowed there.
> Just a general suggestion.

Dropped
> Man pages are absent.
> There is a SHOULD in the guidelines about trying to get them added.
> My understanding is that upstream does not really respond to queries
> so in my opinion, there is no need to take action.
> Of course it is better if you do ask upstream about it.
> Reference:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

Just nice to have.
> > Rpmlint
> >
> > Checking: lunchbox-1.17.0-3.fc35.x86_64.rpm
> >           lunchbox-devel-1.17.0-3.fc35.x86_64.rpm
> >           lunchbox-debuginfo-1.17.0-3.fc35.x86_64.rpm
> >           lunchbox-debugsource-1.17.0-3.fc35.x86_64.rpm
> >           lunchbox-1.17.0-3.fc35.src.rpm
> > lunchbox.x86_64: E: description-line-too-long C Lunchbox is C++ library for multi-threaded programming, providing OS abstraction,
>
> Please shorten it.

Reformatted.