Spec URL: https://ol.fedorapeople.org/mcut.spec SRPM URL: https://ol.fedorapeople.org/mcut-1.3.0-1.fc43.1.src.rpm Description: A library for detecting and resolving intersections between two surface meshes. Fedora Account System Username: ol
This is a dependency for orca-slicer package that is reviewed in bug 2403680.
Copr build: https://copr.fedorainfracloud.org/coprs/build/9688720 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2403985-mcut/fedora-rawhide-x86_64/09688720-mcut/fedora-review/review.txt Found issues: - Not a valid SPDX expression 'LGPL-3.0'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Yes, definitely a false positive. "LGPL-3.0" is valid SPDX identifier: https://spdx.org/licenses/LGPL-3.0.html
(In reply to Oleg Girko from comment #3) > Yes, definitely a false positive. "LGPL-3.0" is valid SPDX identifier: > https://spdx.org/licenses/LGPL-3.0.html It is valid, but deprecated. Please use LGPL-3.0-only or LGPL-3.0-or-later in Fedora instead. This project doesn’t use the usual GNU-prescribed license notice (with the optional “or any later version” clause that would distinguish between the two possibilities), but I do see that the license headers in the source files all say “GNU Lesser General Public License v3+ (LGPL)”, with the + indicating that LGPL-3.0-or-later is intended. There are also other licenses involved. It looks like you should have something like: # The entire source is LGPL-3.0-or-later, except: # - include/mcut/internal/cdt/ is MPL-2.0 # - include/mcut/internal/pool_allocator/ is MIT # # Additionally, include/mcut/internal/nfg/ is also LGPL-3.0-or-later, but with a # different copyright notice. License: LGPL-3.0-or-later AND MPL-2.0 AND MIT You should examine include/mcut/internal/{cdt,nfg,pool_allocator}/ to see if they need to be handled as (partial) bundled dependencies under https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. ---- If you want to repeat the long description text for the devel package, you might like to put it in a macro to avoid duplication: %global common_description %{expand: A library for detecting and resolving intersections between two surface meshes. This project is called "MCUT" (short for 'mesh cutting'), and it provides functionality to perform robust geometry operations between surfaces. The project is designed for a broad range of real-world problems relating to 3D modelling and design tasks. Application areas include computer animation, aerospace and automotive engineering, digital dental modelling, mining, civil and mechanical engineering amongst others.} Then you can write: %description %{common_description} and: %description devel %{common_description} ---- You must not glob over the shared library ABI version / SONAME version like this: %{_libdir}/libmcut.so.* https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files ---- The -devel package needs a fully-versioned, arched dependency on the base package; see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package. Requires: mcut%{?_isa} = %{version}-%{release} You can drop %license LICENSE.txt from the -devel package since it depends on the base package, https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#subpackage-licensing. ---- If at all possible, you should build and run the tests. If this isn’t possible, please add a comment to the spec file explaining why. ---- Patches should have comments explaining what they do, whether they’ve been offered upstream (with a link), or why they need to be downstream-only. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_all_patches_should_have_an_upstream_bug_link_or_comment ---- There’s no reason for the trailing .1 in “Release: 1%{?dist}.1”. These days, it’s recommended to use rpmautospec (%autorelease/%autochangelog), but this isn’t mandatory. https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_release_tag ---- This hasn’t been a full review, even a preliminary one, just the results of a quick skim of the spec file.
(In reply to Ben Beasley from comment #4) > (In reply to Oleg Girko from comment #3) > > Yes, definitely a false positive. "LGPL-3.0" is valid SPDX identifier: > > https://spdx.org/licenses/LGPL-3.0.html > > It is valid, but deprecated. Please use LGPL-3.0-only or LGPL-3.0-or-later > in Fedora instead. This project doesn’t use the usual GNU-prescribed license > notice (with the optional “or any later version” clause that would > distinguish between the two possibilities), but I do see that the license > headers in the source files all say “GNU Lesser General Public License v3+ > (LGPL)”, with the + indicating that LGPL-3.0-or-later is intended. Done. > There are also other licenses involved. It looks like you should have > something like: > > # The entire source is LGPL-3.0-or-later, except: > # - include/mcut/internal/cdt/ is MPL-2.0 Done. > # - include/mcut/internal/pool_allocator/ is MIT > # > # Additionally, include/mcut/internal/nfg/ is also LGPL-3.0-or-later, but > with a > # different copyright notice. There is neither pool_allocator nor ngf in include/mcut/internal in version 1.3.0. They are only in master branch. > License: LGPL-3.0-or-later AND MPL-2.0 AND MIT Done (without MIT that is only in master branch). > You should examine include/mcut/internal/{cdt,nfg,pool_allocator}/ to see if > they need to be handled as (partial) bundled dependencies under > https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. include/mcut/internal/cdt has just a few headers that are used internally and not installed. Although they may be possibly originated in https://github.com/artem-ogre/CDT, Git history shows that they were significantly reworked. So, there's nothing here to unbundle. > If you want to repeat the long description text for the devel package, you > might like to put it in a macro to avoid duplication: Done. > You must not glob over the shared library ABI version / SONAME version like > this: > > %{_libdir}/libmcut.so.* > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_listing_shared_library_files Done. > The -devel package needs a fully-versioned, arched dependency on the base > package; see > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_requiring_base_package. > > Requires: mcut%{?_isa} = %{version}-%{release} Done. > You can drop %license LICENSE.txt from the -devel package since it depends > on the > base package, > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#subpackage-licensing. Done. > If at all possible, you should build and run the tests. If this isn’t > possible, > please add a comment to the spec file explaining why. As tests require a library that is not packaged, I've just added a comment. > Patches should have comments explaining what they do, whether they’ve been > offered > upstream (with a link), or why they need to be downstream-only. > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_all_patches_should_have_an_upstream_bug_link_or_comment Done. > There’s no reason for the trailing .1 in “Release: 1%{?dist}.1”. Please ignore this. This is an artefact of building the package in OBS that requires two numbers in Release. There will be a single number in Fedora package. > These days, it’s recommended to use rpmautospec > (%autorelease/%autochangelog), > but this isn’t mandatory. > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_release_tag I feel more comfortable doing stuff the old-fashioned way.
Updated sources are here. Spec URL: https://ol.fedorapeople.org/mcut.spec SRPM URL: https://ol.fedorapeople.org/mcut-1.3.0-2.fc43.src.rpm
Oops, wrong URL. Spec URL: https://ol.fedorapeople.org/mcut.spec SRPM URL: https://ol.fedorapeople.org/mcut-1.3.0-2.fc44.src.rpm
> # Sorry, no tests for now because they require a library > # that is not packaged yetL > # https://github.com/sheredom/utest.h Here, I’ve submitted it: bug 2412239
Now that I’ve packaged utest, you should be able to run the tests. This project wants to download the header at build time, # # Download and unpack utest at configure time # FetchContent_Populate( utest GIT_REPOSITORY https://github.com/sheredom/utest.h.git GIT_TAG master GIT_PROGRESS FALSE ) set(utest_include_dir ${utest_SOURCE_DIR}) […] target_include_directories(mcut_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include ${MCUT_INCLUDE_DIR} ${utest_include_dir} ${mio_include_dir}) but it is always used as #include "utest.h", so a small build-system patch to remove the call to FetchContent_Populate() and the use of utest_include_dir, along with "BuildRequires: utest-static" (due to guidelines on header-only libraries) in the spec file, would be sufficient to get this working. (There might be some hackery you can do with options for FetchContent and putting files in particular places without patching CMakeLists.txt, but the suggested small patch is probably much easier.)