Spec URL: https://gui1ty.fedorapeople.org/review/units-llnl.spec SRPM URL: https://gui1ty.fedorapeople.org/review/units-llnl-0.12.3-3.fc43.src.rpm Description: The Units library provides a means of working with units of measurement at runtime, including conversion to and from strings. It provides a small number of types for working with units and measurements and operations necessary for user input and output with units. Fedora Account System Username: gui1ty
Copr build: https://copr.fedorainfracloud.org/coprs/build/8820905 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2354972-units-llnl/fedora-rawhide-x86_64/08820905-units-llnl/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Upstream implemented SOVERSION as well as allowing use of external (to the project) GTest, making my patches obsolete. The failing Python test has also been fixed. Spec URL: https://gui1ty.fedorapeople.org/review/units-llnl.spec SRPM URL: https://gui1ty.fedorapeople.org/review/units-llnl-0.13.0-1.fc43.src.rpm
Created attachment 2083923 [details] The .spec file difference from Copr build 8820905 to 8874744
Copr build: https://copr.fedorainfracloud.org/coprs/build/8874744 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2354972-units-llnl/fedora-rawhide-x86_64/08874744-units-llnl/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Started reviewing this now. To begin with, I noticed that the sources include a "ThirdParty" folder which bundles bits like tinyxml2. We should either make it use system libraries or update the spec to note the bundled bits. If we bundle, the license will also need to include the licenses of the bundled bits (if different from the package), since they're being linked to in the build. units-0.13.0/ThirdParty/ units-0.13.0/ThirdParty/CLI11.hpp units-0.13.0/ThirdParty/cmake/ units-0.13.0/ThirdParty/cmake/CodeCoverage.cmake units-0.13.0/ThirdParty/cmake/GitUtils.cmake units-0.13.0/ThirdParty/cmake/Utils.cmake units-0.13.0/ThirdParty/cmake/extraMacros.cmake units-0.13.0/ThirdParty/cmake/updateGitSubmodules.cmake units-0.13.0/ThirdParty/googletest/ units-0.13.0/ThirdParty/json.hpp units-0.13.0/ThirdParty/xml/ units-0.13.0/ThirdParty/xml/tinyxml2.cpp units-0.13.0/ThirdParty/xml/tinyxml2.h
I admit I totally overlooked those. However, the library itself does not depend on anything in the ThirdParty directory: # ldd libunits.so.0.13.0 linux-vdso.so.1 (0x00007f72e78fd000) libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f72e7400000) libm.so.6 => /lib64/libm.so.6 (0x00007f72e777e000) libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f72e7752000) libc.so.6 => /lib64/libc.so.6 (0x00007f72e720f000) /lib64/ld-linux-x86-64.so.2 (0x00007f72e78ff000) nor are any of the files shipped in the final RPMs. The stuff inside ThirdParty is exclusively used for the test suite. Would it be sufficient to make a note of the sources and their licenses in the spec file? That would be less hassle than having to patch the CMake configuration and the test sources. On the other hand, we do have all the required packages available in Fedora: CLI11.hpp --> cli11 json.hpp --> json xml/tinxml2.cpp --> tinyxml2 (not part of the devel package; might be modified) xml/tinyxml2.h --> tinyxml2 units-0.13.0/ThirdParty/googletest/ is a submodule stub. It has already been made configurable and I'm using the Fedora package.
The guidelines say: https://docs.fedoraproject.org/en-US/legal/license-field/#_basic_rule "... License: tag expression should be an enumeration of all licenses found in the source code of the package, but excluding any licenses that cover material in the source code that is not copied into the binary RPM(s), either verbatim or transformed in some way (for example, by compilation)." So, in this case, since the third party bits aren't ending up in the binary, we don't need to include their licenses etc. nor specify them as bundled. I think we should add a note in the spec about this, and a reminder that this should be re-checked for every new release, just in case upstream does start to use them to build the library etc. I'll complete the review in the next couple of days. Cheers,
Thanks for doing the review. I have an update regarding the included third party stuff. I turns out CLI11.hpp is included in converter/converter.cpp, which is the source for units_convert - an app provided for doing standalone unit conversion, which I also intent to include in the binary RPM. I updated the spec file to reflect on the situation regarding the shipped third party sources. I also removed the shipped CLI11.hpp in favor of the headers from cli11-devel. CLI11 is a header only library. Now I wonder if "transformed in some way (for example, by compilation)" applies to header files. If not, unbundling the header file would not strictly be required. But with this change, this becomes a question of curiosity. For quick reference, upstream has it documented in https://github.com/LLNL/units/blob/main/CONTRIBUTORS.md (a bit of a misnomer). Spec URL: https://gui1ty.fedorapeople.org/review/units-llnl.spec SRPM URL: https://gui1ty.fedorapeople.org/review/units-llnl-0.13.0-2.fc43.src.rpm
Created attachment 2084489 [details] The .spec file difference from Copr build 8874744 to 8892748
Copr build: https://copr.fedorainfracloud.org/coprs/build/8892748 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2354972-units-llnl/fedora-rawhide-x86_64/08892748-units-llnl/fedora-review/review.txt Please take a look if any issues were found. --- 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.
(In reply to Sandro from comment #8) > Now I wonder if "transformed in some way (for example, by compilation)" > applies to header files. If not, unbundling the header file would not > strictly be required. But with this change, this becomes a question of > curiosity. > My understanding is that during compilation, the compiler replaces the contents of the `#include` statements with the complete contents of the referred files. So, header-only libraries (the used bits) will end up being compiled and as part of the generated binaries, which means their licensing needs to be taken into account. Does that make sense?
Yes, that makes sense. That was my first intuition as well. Luckily, the headers are available in Fedora and replacing the shipped header with the system installed header wasn't very hard.
That looks good to me. XXX APPROVED XXX - A new version was released a couple of weeks ago. Please do update before import. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Dist tag is present. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. [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]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. 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. [x]: 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 License", "*No copyright* Boost Software License", "*No copyright* BSD 3-Clause License", "MIT License", "BSD 3-Clause License and/or Boost Software License 1.0", "*No copyright* zlib License". 444 files have unknown license. Detailed output of licensecheck in /home/asinha/dump/fedora- reviews/2354972-units-llnl/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [-]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib64/python3.13, /usr/lib64/python3.13/site-packages [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [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. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 66917 bytes in 6 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: The License field must be a valid SPDX expression. [x]: Package requires other packages for directories it uses. [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]: 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]: Packages must not store files under /srv, /opt or /usr/local Python: [x]: Python eggs must not download any dependencies during the build process. [-]: A package which is used by another package via an egg interface should provide egg info. [x]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [x]: Packages MUST NOT have dependencies (either build-time or runtime) on packages named with the unversioned python- prefix unless no properly versioned package exists. Dependencies on Python packages instead MUST use names beginning with python2- or python3- as appropriate. [x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files [x]: Binary eggs must be removed in %prep ===== SHOULD items ===== Generic: [-]: 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. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in units- llnl-devel , python3-units-llnl ^ These lines are there, false positive. [x]: Package functions as described. [?]: Latest version is packaged. ^ A new version was released a couple of weeks ago. Please update before import. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [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]: 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: [!]: Spec file according to URL is the same as in SRPM. Note: Spec file as given by url is not the same as in SRPM (see attached diff). See: (this test has no URL) [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: units-llnl-0.13.0-2.fc43.x86_64.rpm units-llnl-devel-0.13.0-2.fc43.x86_64.rpm python3-units-llnl-0.13.0-2.fc43.x86_64.rpm units-llnl-0.13.0-2.fc43.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.7.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpujs9koqf')] checks: 32, packages: 4 units-llnl.x86_64: W: no-manual-page-for-binary units_convert units-llnl-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 2 warnings, 29 filtered, 0 badness; has taken 0.4 s Rpmlint (debuginfo) ------------------- Checking: units-llnl-debuginfo-0.13.0-2.fc43.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.7.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmp6_epg8f7')] checks: 32, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 12 filtered, 0 badness; has taken 0.2 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.7.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 4 units-llnl.x86_64: W: no-manual-page-for-binary units_convert units-llnl-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 2 warnings, 39 filtered, 0 badness; has taken 0.4 s Unversioned so-files -------------------- python3-units-llnl: /usr/lib64/python3.13/site-packages/units_llnl/units_llnl_ext.abi3.so Source checksums ---------------- https://github.com/LLNL/units/archive/v0.13.0/units-0.13.0.tar.gz : CHECKSUM(SHA256) this package : 7d6e8bfb59085448eff093d009dbbe45ea3ddca31f9fcb290fde49b141a87227 CHECKSUM(SHA256) upstream package : 7d6e8bfb59085448eff093d009dbbe45ea3ddca31f9fcb290fde49b141a87227 Requires -------- units-llnl (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libm.so.6()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.15)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) libunits.so.0()(64bit) rtld(GNU_HASH) units-llnl-devel (rpmlib, GLIBC filtered): cmake-filesystem(x86-64) libunits.so.0()(64bit) units-llnl(x86-64) python3-units-llnl (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libm.so.6()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.11)(64bit) libstdc++.so.6(CXXABI_1.3.13)(64bit) libstdc++.so.6(CXXABI_1.3.15)(64bit) libstdc++.so.6(CXXABI_1.3.3)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) libunits.so.0()(64bit) python(abi) rtld(GNU_HASH) units-llnl(x86-64) Provides -------- units-llnl: libunits.so.0()(64bit) units-llnl units-llnl(x86-64) units-llnl-devel: cmake(units) units-llnl-devel units-llnl-devel(x86-64) python3-units-llnl: python-units-llnl python3-units-llnl python3-units-llnl(x86-64) python3.13-units-llnl python3.13dist(units-llnl) python3dist(units-llnl) Diff spec file in url and in SRPM --------------------------------- --- /home/asinha/dump/fedora-reviews/2354972-units-llnl/srpm/units-llnl.spec 2025-04-28 14:32:01.993537286 +0100 +++ /home/asinha/dump/fedora-reviews/2354972-units-llnl/srpm-unpacked/units-llnl.spec 2025-04-11 01:00:00.000000000 +0100 @@ -1,2 +1,12 @@ +## START: Set by rpmautospec +## (rpmautospec version 0.7.3) +## RPMAUTOSPEC: autorelease, autochangelog +%define autorelease(e:s:pb:n) %{?-p:0.}%{lua: + release_number = 2; + base_release_number = tonumber(rpm.expand("%{?-b*}%{!?-b:1}")); + print(release_number + base_release_number - 1); +}%{?-e:.%{-e*}}%{?-s:.%{-s*}}%{!?-n:%{?dist}} +## END: Set by rpmautospec + %global pypi_name units-llnl %bcond python 1 @@ -130,3 +140,18 @@ %changelog -%autochangelog +## START: Generated by rpmautospec +* Fri Apr 11 2025 Sandro <devel> - 0.13.0-2 +- Clarify third party sources and replace header + +* Tue Apr 08 2025 Sandro <devel> - 0.13.0-1 +- Update to 0.13.0 + +* Tue Mar 25 2025 Sandro <devel> - 0.12.3-3 +- Add Python module using shared lib + +* Tue Mar 25 2025 Sandro <devel> - 0.12.3-2 +- Fix build of shared library and tests + +* Sun Mar 23 2025 Sandro <devel> - 0.12.3-1 +- Initial package +## END: Generated by rpmautospec Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/sbin/fedora-review -b 2354972 Buildroot used: fedora-rawhide-x86_64 Active plugins: Python, Generic, C/C++, Shell-api Disabled plugins: PHP, R, Ocaml, SugarActivity, fonts, Java, Perl, Haskell Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH
The Pagure repository was created at https://src.fedoraproject.org/rpms/units-llnl
Thanks for the review! > A new version was released a couple of weeks ago. Please do update before import. I will push version 0.13.1.
FEDORA-2025-a4726f49b8 (units-llnl-0.13.1-1.fc43) has been submitted as an update to Fedora 43. https://bodhi.fedoraproject.org/updates/FEDORA-2025-a4726f49b8
FEDORA-2025-a4726f49b8 (units-llnl-0.13.1-1.fc43) has been pushed to the Fedora 43 stable repository. If problem still persists, please make note of it in this bug report.