Spec URL: https://github.com/intel/qatlib/releases/download/20.08.0/qatlib.spec SRPM URL: https://github.com/intel/qatlib/releases/download/20.08.0/qatlib-20.08.0-0.1.fc32.src.rpm Description: Intel(R) QuickAssist Technology (Intel(R) QAT) provides hardware acceleration for offloading security, authentication and compression services from the CPU, thus significantly increasing the performance and efficiency of standard platform solutions. Its services include symmetric encryption and authentication, asymmetric encryption, digital signatures, RSA, DH and ECC, and lossless data compression. This package provides user space libraries that allow access to Intel(R) QuickAssist devices and expose the Intel(R) QuickAssist APIs. Fedora Account System Username: gcabiddu
I'll handle this review.
I tried to run fedora-review on this, but it failed to build (see the item below about missing build requirements). Here is a partial manual review of what I've noticed so far. - Using 0.1 for the Release field is fine during the review, but it should be raised to 1 before being imported into dist-git. After that it should always be 1 or higher [0], unless packaging a prerelease version or git snapshot. - Remove all instances of `(R)` from the Summary field and the %description sections [1]. - The INSTALL file indicates a complicated licensing situation [2]. All of these licenses must be reflected in the License field, using the combined scenario guidelines [3]. I also noticed that the INSTALL file mentions a LICENSE.GPL file, but that does not exist upstream. Could you assist in getting it added? - Source0 is not following the recommended format [4]. It should look like this: https://github.com/intel/%{name}/archive/%{version}/%{name}-%{version}.tar.gz - There are multiple missing build requirements. I can tell at least these are missing: autoconf automake libtool systemd-devel openssl-devel zlib-devel - RPM will automatically adds requirements for several glibc virtual provides, so the explicit requires for glibc is redundant and must be removed [5]. - The requires for /sbin/ldconfig and invoking that during the %post/%preun/%postun scriptlets should be removed [6]. - The devel package's requirement on the base package must be arch-specific [7]. - Rather than conditionally running autogen.sh during %build, it would be better to always run `autoreconf -vif` during %prep. - There is a lot going on during %install. Is there a Makefile target we could use instead to improve spec file legibility [8]? - The %pre scriptlet should use the template for dynamic allocation [9]. - Man pages must be referenced with a wildcard pattern to allow RPM to use its preferred compression format (which may not be gz in the future) [10]. - The `%files devel` section can be trimmed down by using just `%{_includedir}/qat` (which is recursive), rather than the directory and globbing all the files in the directory. - The version in the changelog entry (2010u) doesn't match the Version field. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simple_versioning [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_trademarks_in_summary_or_description [2] https://github.com/intel/qatlib/blob/20.08.0/INSTALL#L33-L46 [3] https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_combined_dual_and_multiple_licensing_scenario [4] https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags [5] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_requires [6] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries [7] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package [8] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_legibility [9] https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_dynamic_allocation [10] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages
I want to point out one thing that was brought up in bug 1885495. Adding a license in a comment of the spec file is only appropriate if you wish for the spec file itself to be available under a different license than the default MIT license specified by the FPCA [0]. This does not have to match the software being packaged. I'd recommend removing it as well for simplicity, but it's not strictly required. [0] https://fedoraproject.org/wiki/Licensing:Main#License_of_Fedora_SPEC_Files
Thanks for the review Carl. I uploaded the new spec at the same location. More comments below. > I tried to run fedora-review on this, but it failed to build (see the item below about missing build requirements). Here is a partial manual review of what I've noticed so far. > > - Using 0.1 for the Release field is fine during the review, but it should be raised to 1 before being imported into dist-git. After that it should always be 1 or higher [0], unless packaging a prerelease version or git snapshot. Done. The Release field has been changed to 1%{?dist}. > - Remove all instances of `(R)` from the Summary field and the %description sections [1]. Done. > - The INSTALL file indicates a complicated licensing situation [2]. All of these licenses must be reflected in the License field, using the combined scenario guidelines [3]. I also noticed that the INSTALL file mentions a LICENSE.GPL file, but that does not exist upstream. Could you assist in getting it added? The license of the project is BSD. The library that we are packaging uses (1) some Intel code that is BSD-3-Clause, (2) some Intel code which is dual license (BSD-3-Clause OR GPL-2.0-only) and (3) portions of OpenSSL. The sample codes, which are not included in this RPM but are built by default, use Intel code which is dual lincese (BSD-3-Clause OR GPL-2.0-only). These link against both OpenSSL and Zlib. Should the License field stay as BSD or reflect the internal components? Regarding the INSTALL file in [2], it is not correct. In this project we don't have code that is only GPLv2. We will be updating this file in the next release of the library. > - Source0 is not following the recommended format [4]. It should look like this: > > https://github.com/intel/%{name}/archive/%{version}/%{name}-%{version}.tar.gz Fixed. > - There are multiple missing build requirements. I can tell at least these are missing: > > autoconf automake libtool systemd-devel openssl-devel zlib-devel Fixed. > > - RPM will automatically adds requirements for several glibc virtual provides, so the explicit requires for glibc is redundant and must be removed [5]. Removed Glibc requirement. > - The requires for /sbin/ldconfig and invoking that during the %post/%preun/%postun scriptlets should be removed [6]. Removed /sbin/ldconfig requirement. > - The devel package's requirement on the base package must be arch-specific [7]. Fixed. > - Rather than conditionally running autogen.sh during %build, it would be better to always run `autoreconf -vif` during %prep. Fixed. > - There is a lot going on during %install. Is there a Makefile target we could use instead to improve spec file legibility [8]? Will be done as part of the next release of QATlib (20.10). We need to go through our internal release process in order to change the content of the repository. > - The %pre scriptlet should use the template for dynamic allocation [9]. Done > - Man pages must be referenced with a wildcard pattern to allow RPM to use its preferred compression format (which may not be gz in the future) [10]. Done. > - The `%files devel` section can be trimmed down by using just `%{_includedir}/qat` (which is recursive), rather than the directory and globbing all the files in the directory. Done. > - The version in the changelog entry (2010u) doesn't match the Version field. Fixed. > Adding a license in a comment of the spec file is only appropriate if you wish for the spec file itself to be available under a different license than the default MIT license specified by the FPCA [0]. This does not have to match the software being packaged. I'd recommend removing it as well for simplicity, but it's not strictly required. I need to consult the legal team to check if I can remove the license.
> I uploaded the new spec at the same location. The review tool doesn't work with that link. It requires the spec URL to return raw text. It also requires the current SRPM. Please upload the latest spec file and SRPM somewhere online, and make a new comment that follows the initial template. Spec URL: <spec info here> SRPM URL: <srpm info here> > The license of the project is BSD. The library that we are packaging uses (1) some Intel code that is BSD-3-Clause, (2) some Intel code which is dual license (BSD-3-Clause OR GPL-2.0-only) and (3) portions of OpenSSL. > The sample codes, which are not included in this RPM but are built by default, use Intel code which is dual lincese (BSD-3-Clause OR GPL-2.0-only). These link against both OpenSSL and Zlib. > Should the License field stay as BSD or reflect the internal components? > Regarding the INSTALL file in [2], it is not correct. In this project we don't have code that is only GPLv2. We will be updating this file in the next release of the library. The license field must reflect ALL of the licenses of the distributed software, including anything regarded as "internal". Libraries that are dynamically linked do not need to be referenced. Libraries that are bundled should be removed in favor of dynamically linking against the system copies. If this is not possible, the license field must also reference the license of those bundled libraries. There must also be special provides directives for the bundled libraries. Reference the guidelines [0] on how to handle bundled libraries correctly. Based on what you are describing, if you can debundled the OpenSSL parts, I think the correct license text should be: License: BSD and (BSD or GPLv2) Please review the license guidelines [1] yourself to ensure this matches the state of the software. > > - The %pre scriptlet should use the template for dynamic allocation [9]. > Done This is still missing the scriptlet requirement on shadow-utils. Add this line near the other requirements in the spec file: Requires(pre): shadow-utils > > - The `%files devel` section can be trimmed down by using just `%{_includedir}/qat` (which is recursive), rather than the directory and globbing all the files in the directory. > Done. The redundant `%dir %{_includedir}/qat` line is still there, please remove it. > > - The version in the changelog entry (2010u) doesn't match the Version field. > Fixed. While fixing the version, the release was dropped. Please add it back. It should look like this: * Fri Oct 16 2020 Giovanni Cabiddu <giovanni.cabiddu> - 20.08.0-1 > I need to consult the legal team to check if I can remove the license. For reference, there is already plenty of Intel software in Fedora under various licenses that keep their respective spec file under the default MIT license. Some examples: - intel-clear-sans-fonts (ASL 2.0) [2] - intel-gmmlib (MIT and BSD) [3] - intel-mediasdk (MIT) [4] - intel-mpi-benchmarks (CPL) [5] - intel-undervolt (GPLv3+) [6] - intelhex (BSD) [7] [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/ [2] https://src.fedoraproject.org/rpms/intel-clear-sans-fonts/blob/master/f/intel-clear-sans-fonts.spec [3] https://src.fedoraproject.org/rpms/intel-gmmlib/blob/master/f/intel-gmmlib.spec [4] https://src.fedoraproject.org/rpms/intel-mediasdk/blob/master/f/intel-mediasdk.spec [5] https://src.fedoraproject.org/rpms/intel-mpi-benchmarks/blob/master/f/intel-mpi-benchmarks.spec [6] https://src.fedoraproject.org/rpms/intel-undervolt/blob/master/f/intel-undervolt.spec [7] https://src.fedoraproject.org/rpms/intelhex/blob/master/f/intelhex.spec
Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib-20.08.0-1.fc32.src.rpm >The license field must reflect ALL of the licenses of the distributed software, including anything regarded as "internal". Libraries that are dynamically linked do not need to be referenced. Libraries that are bundled should be removed in favor of dynamically linking against the system copies. If this is not possible, the license field must also reference the license of those bundled libraries. There must also be special provides directives for the bundled libraries. Reference the guidelines [0] on how to handle bundled libraries correctly. Based on what you are describing, if you can debundled the OpenSSL parts, I think the correct license text should be: > > License: BSD and (BSD or GPLv2) I updated the License to: License: BSD and (BSD or GPLv2) and OpenSSL as the code contains some snippets of OpenSSL libcrypto. >This is still missing the scriptlet requirement on shadow-utils. Add this line near the other requirements in the spec file: > > Requires(pre): shadow-utils Done. >The redundant `%dir %{_includedir}/qat` line is still there, please remove it. Done. >While fixing the version, the release was dropped. Please add it back. It should look like this: > > * Fri Oct 16 2020 Giovanni Cabiddu <giovanni.cabiddu> - 20.08.0-1 Done. On this, what level of detail would you like to see in the changelog for this phase? Should we just have a single entry for the initial version of the package? >> I need to consult the legal team to check if I can remove the license. > >For reference, there is already plenty of Intel software in Fedora under various licenses that keep their respective spec file under the default MIT license. I'm trying to understand internally what are the steps to change the license for this file as the project was approved as BSD.
> as the code contains some snippets of OpenSSL libcrypto. If this package is going to bundle openssl (even if only partially), there are two MUST requirements [0]. 1. Add the line `Provides: bundled(openssl) = <version>`. 2. Open an issue upstream to request it be possible to build against the system openssl, and add a link to that issue as a comment in the spec file. On a related note, running rpmlint on the installed package generates the following warnings. qatlib.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libqat.so.0.20.08.0 /lib64/libcrypto.so.1.1 qatlib.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libusdm.so.0.20.08.0 /lib64/libcrypto.so.1.1 The wiki has some notes on how to resolve this [1]. > On this, what level of detail would you like to see in the changelog for this phase? > Should we just have a single entry for the initial version of the package? It's ok to consolidate all of the review work into a single changelog entry for the initial package, or you can keep each round of review changes as a separate entry. Your choice. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling [1] https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency
I uploaded a new version of the SPEC and the RPM: Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib-20.08.0-1.fc32.src.rpm > If this package is going to bundle openssl (even if only partially), there are two MUST requirements [0]. > > 1. Add the line `Provides: bundled(openssl) = <version>`. Based on [0] I added `Provides: bundled(libcrypto) = 1.1.1c`. If `openssl` is preferred I can change it. > 2. Open an issue upstream to request it be possible to build against the system openssl, and add a link to that issue as a comment in the spec file. Done. Opened a ticket [1] and added a comment before the line `Provides:`. A note on this. The reason why qatlib includes an implementation of MD5, SHA and AES [2] is to avoid a potential circular dependency when QAT OpenSSL Engine [3] is in the system. We are looking at a ways to remove the bundled libcrypto in qatlib and this change might be part of the next release. > On a related note, running rpmlint on the installed package generates the following warnings. > > qatlib.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libqat.so.0.20.08.0 /lib64/libcrypto.so.1.1 > qatlib.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libusdm.so.0.20.08.0 /lib64/libcrypto.so.1.1 > > The wiki has some notes on how to resolve this [1]. Done. Regarding the license on the spec file, I checked internally. We can change the license from BSD to MIT however we cannot remove the header on the file. One question regarding the Arch. Qatlib was tested and validated only on x86_64 platforms. Should we add `ExclusiveArch: x86_64` as described in [4] or mention explicitly the architectures where the build does not work as described in [5]? [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling [1] https://github.com/intel/qatlib/issues/2 [2] https://github.com/intel/qatlib/tree/main/quickassist/utilities/osal/src/linux/user_space/openssl [3] https://github.com/intel/QAT_Engine [4] https://fedoraproject.org/wiki/Architectures#ExcludeArch_.26_ExclusiveArch [5] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures
> Based on [0] I added `Provides: bundled(libcrypto) = 1.1.1c`. If `openssl` is preferred I can change it. Yes please, the guidelines are clear here. "If the bundled package also exists separately in the distribution, use the name of that package." Use `Provides: bundled(openssl) = 1.1.1c`. The idea is that if there is a vulnerability discovered in a package, we can query for everything that provides it as a bundled library. That only works when we standardize on a name so we don't have to query for every possible variation or sub-library of that package. > The reason why qatlib includes an implementation of MD5, SHA and AES [2] is to avoid a potential circular dependency when QAT OpenSSL Engine [3] is in the system. I'm not sure I follow. Those are provided by the system openssl library, not by qatengine. Where does it become circular? If it's debundled, the build order for the distribution would be openssl, qatlib, then qatengine. > Regarding the license on the spec file, I checked internally. We can change the license from BSD to MIT however we cannot remove the header on the file. That's odd, because qatengine got permission to remove their license header (bug 1885495 comment 10). If the spec file will be under the MIT license (the Fedora default for spec files), then the comment header is just noise and needs to be removed to improve legibility. To be clear, the qatlib.spec.in file in the upstream repo can keep the header, we're only discussing the spec file that will be imported into Fedora package sources. > One question regarding the Arch. Qatlib was tested and validated only on x86_64 platforms. Should we add `ExclusiveArch: x86_64` as described in [4] or mention explicitly the architectures where the build does not work as described in [5]? The latter. There are many upstreams who don't test on alternative architectures. Fedora basically is their alternative arch testing and QA (which works better when there is an upstream test suite to run in %check). Unless it has been established as fact that the software doesn't compile, build, or work on an architecture, the package should build for that architecture.
I uploaded a new version of the SPEC and the RPM: Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib-20.08.0-1.fc33.src.rpm >Yes please, the guidelines are clear here. "If the bundled package also exists separately in the distribution, use the name of that package." Use `Provides: bundled(openssl) = 1.1.1c`. The idea is that if there is a vulnerability discovered in a package, we can query for everything that provides it as a bundled library. That only works when we standardize on a name so we don't have to query for every possible variation or sub-library of that package. Done. >I'm not sure I follow. Those are provided by the system openssl library, not by qatengine. Where does it become circular? If it's debundled, the build order for the distribution would be openssl, qatlib, then qatengine. The dependency is a run time, if qatlib uses the OpenSSL EVP API. In that case we would have: app -> openssl:libcrypto_EVP -> qat_engine -> qat_lib -> openssl:libcrypto_EVP -> qat_engine -> qat_lib -> REPEAT We resolved it by linking against OpenSSL/libcrypto and using the lower level (public) crypto APIs for the algorithms we need. This change is included in the next release. This will work for now, however we have to re-think at the approach for the future since for OpenSSL 3.0 those APIs are marked as DEPRECATED. E.g. https://github.com/openssl/openssl/blob/master/include/openssl/aes.h#L51 >That's odd, because qatengine got permission to remove their license header (bug 1885495 comment 10). If the spec file will be under the MIT license (the Fedora default for spec files), then the comment header is just noise and needs to be removed to improve legibility. To be clear, the qatlib.spec.in file in the upstream repo can keep the header, we're only discussing the spec file that will be imported into Fedora package sources. I checked again and the guideline is to still leave the license on the file. In order to address the noise/visual clutter in the file we replaced the full header with the SPDX License Identifier: `# SPDX-License-Identifier: MIT` Is this ok? >The latter. There are many upstreams who don't test on alternative architectures. Fedora basically is their alternative arch testing and QA (which works better when there is an upstream test suite to run in %check). Unless it has been established as fact that the software doesn't compile, build, or work on an architecture, the package should build for that architecture. Thanks. I haven't run the build yet any alternative architectures. Is it something I have to do? If yes, which architectures should I try?
I uploaded a new version of the SPEC and the RPM: Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_08/rpm/qatlib-20.08.0-1.fc33.src.rpm I attempted a build in copr (https://copr.fedorainfracloud.org/coprs/gcabiddu/qatlib/build/1745809/) for the following targets: - centos-stream-aarch64 - centos-stream-x86_64 - fedora-eln-aarch64 - fedora-eln-s390x - fedora-eln-x86_64 - fedora-rawhide-aarch64 - fedora-rawhide-s390x - fedora-rawhide-x86_64 As I foreseen the build fails on aarch64 and s390x. I excluded those archs in the new revision of the spec. The build is also failing for fedora-eln-x86_64, however the failure seems to be related to a dnf problem not related to qatlib.
> app -> openssl:libcrypto_EVP -> qat_engine -> qat_lib -> openssl:libcrypto_EVP -> qat_engine -> qat_lib -> REPEAT I still don't understand this, but I'm admittedly not a crypto expert. Regardless, if it's only possible for this to function with bundled libcrypto, it is permissible for the package to do that. We've got the bundled library provides in place, so we are covered. > I checked again and the guideline is to still leave the license on the file. In order to address the noise/visual clutter in the file we replaced the full header with the SPDX License Identifier: `# SPDX-License-Identifier: MIT` > Is this ok? It's still redundant, but it's a big improvement and I'll take it. > Thanks. I haven't run the build yet any alternative architectures. Is it something I have to do? If yes, which architectures should I try? When you build the package in Fedora it will attempt to build on all architectures, unless they are excluded. I took your copr srpm and submitted it as a scratch build in Fedora's build system with this command: koji build --scratch rawhide qatlib-20.08.0-1.fc34.src.rpm Here is the result where you can see all the architectures it attempted to build: https://koji.fedoraproject.org/koji/taskinfo?taskID=55010398 Based on that, I think the appropriate exclusion will look like this: ExcludeArch: %{arm} aarch64 %{power64} s390x Remember after review you'll need to open bugzillas for each of those, mark them as blocking the tracker bugs listed in the guidelines [0], and include links to each of them in spec file comments. Other than that slight adjustment to the ExcludeArch tag, I think this looks good. I'm running fedora-review on it now. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures
The automated review looks mostly good, just a two items left that need to be fixed. 1. Update the ExcludeArch tag as described in comment 12. 2. Add a comments explaining the license breakdown. Some examples are given in the guidelines [0]. Those are simple enough that I won't hold up the review any longer. Just fix them prior to importing the SRPM to distgit. PACKAGE APPROVED. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== 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. [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 "New" or "Revised" License", "BSD 4-clause "Original" or "Old" License Apache License 1.0", "BSD 3-clause "New" or "Revised" License GNU General Public License, Version 2", "OpenSSL License BSD 3-clause "New" or "Revised" License". 4 files have unknown license. Detailed output of licensecheck in /home/carl/packaging/reviews/1885430-qatlib/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]: %build honors applicable compiler flags or justifies otherwise. [!]: Package contains no bundled libraries without FPC exception. Note: Bundled is now permitted by guidelines. [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. [x]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [!]: Package is not known to require an ExcludeArch tag. Note: Failing arches noted in ExcludeArch and will have bugs opened after package is imported. [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]: 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]: systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in qatlib [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: [-]: 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]: Package functions as described. [x]: Latest version is packaged. [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. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [!]: Package should compile and build into binary rpms on all supported architectures. Note: Failing arches noted in ExcludeArch and will have bugs opened after package is imported. [-]: %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]: Fully versioned dependency in subpackages if applicable. [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: [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. [x]: Package should not use obsolete m4 macros [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: qatlib-20.08.0-1.fc34.x86_64.rpm qatlib-devel-20.08.0-1.fc34.x86_64.rpm qatlib-debuginfo-20.08.0-1.fc34.x86_64.rpm qatlib-debugsource-20.08.0-1.fc34.x86_64.rpm qatlib-20.08.0-1.fc34.src.rpm qatlib.x86_64: E: postin-without-ldconfig /usr/lib64/libqat.so.0.20.08.0 qatlib.x86_64: E: postun-without-ldconfig /usr/lib64/libqat.so.0.20.08.0 qatlib.x86_64: E: postin-without-ldconfig /usr/lib64/libusdm.so.0.20.08.0 qatlib.x86_64: E: postun-without-ldconfig /usr/lib64/libusdm.so.0.20.08.0 qatlib-devel.x86_64: W: no-documentation 5 packages and 0 specfiles checked; 4 errors, 1 warnings. Rpmlint (debuginfo) ------------------- Checking: qatlib-debuginfo-20.08.0-1.fc34.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- qatlib.x86_64: W: invalid-url URL: https://github.com/intel/qatlib <urlopen error [Errno -3] Temporary failure in name resolution> qatlib.x86_64: E: postin-without-ldconfig /usr/lib64/libqat.so.0.20.08.0 qatlib.x86_64: E: postun-without-ldconfig /usr/lib64/libqat.so.0.20.08.0 qatlib.x86_64: E: postin-without-ldconfig /usr/lib64/libusdm.so.0.20.08.0 qatlib.x86_64: E: postun-without-ldconfig /usr/lib64/libusdm.so.0.20.08.0 qatlib-devel.x86_64: W: invalid-url URL: https://github.com/intel/qatlib <urlopen error [Errno -3] Temporary failure in name resolution> qatlib-devel.x86_64: W: no-documentation qatlib-debugsource.x86_64: W: invalid-url URL: https://github.com/intel/qatlib <urlopen error [Errno -3] Temporary failure in name resolution> qatlib-debuginfo.x86_64: W: invalid-url URL: https://github.com/intel/qatlib <urlopen error [Errno -3] Temporary failure in name resolution> 4 packages and 0 specfiles checked; 4 errors, 5 warnings. Source checksums ---------------- https://github.com/intel/qatlib/archive/20.08.0/qatlib-20.08.0.tar.gz : CHECKSUM(SHA256) this package : b94d00a5e3198eaca7fcd3a4caf2b266e8db712eadb639545b59901f11aba12e CHECKSUM(SHA256) upstream package : b94d00a5e3198eaca7fcd3a4caf2b266e8db712eadb639545b59901f11aba12e Requires -------- qatlib (rpmlib, GLIBC filtered): /bin/sh /usr/bin/sh libc.so.6()(64bit) libpthread.so.0()(64bit) libusdm.so.0()(64bit) rtld(GNU_HASH) shadow-utils systemd qatlib-devel (rpmlib, GLIBC filtered): libqat.so.0()(64bit) libusdm.so.0()(64bit) qatlib(x86-64) qatlib-debuginfo (rpmlib, GLIBC filtered): qatlib-debugsource (rpmlib, GLIBC filtered): Provides -------- qatlib: bundled(openssl) libqat.so.0()(64bit) libusdm.so.0()(64bit) qatlib qatlib(x86-64) qatlib-devel: qatlib-devel qatlib-devel(x86-64) qatlib-debuginfo: debuginfo(build-id) qatlib-debuginfo qatlib-debuginfo(x86-64) qatlib-debugsource: qatlib-debugsource qatlib-debugsource(x86-64)
I just sponsored you as well. Let me know if you have any issues on the next steps. https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Thanks Carl. Two things: - We just released 20.10 which includes the removal of bundled OpenSSL/libcrypto and changes to the install target in the Makefile that allow to improve spec file legibility. Do I import to dist-git the new version of the spec or the version that matches 20.08 and then we update to 20.10? Here are the new spec and src rpm if you want to have a look: Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib-20.10.0-1.fc33.src.rpm - When creating a ticket for tracking ExcludeArch, there isn't a component yet for qatlib. Is this created when the repo gets created or there is something I should do?
Awesome, thanks for doing the work to get that debundled and add the makefile target. Importing the latest version of the spec file into dist-git is fine. However, in the latest version of the spec file you made an extra change that needs to be reverted before it's imported. -%{_libdir}/libqat.so.%{soversion}.%{version} -%{_libdir}/libusdm.so.%{soversion}.%{version} -%{_libdir}/libqat.so.%{soversion} -%{_libdir}/libusdm.so.%{soversion} +%{_libdir}/libqat.so.* +%{_libdir}/libusdm.so.* When listing shared library files, they should not be listed with a glob that conceals the soname version [0]. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files
I tried a build reverting that myself, and something else looks off. Previously the build was creating the library files with this suffix: .so.%{soversion}.%{version} Now using the Makefile target it creates them with this suffix: .so.%{version} Is that a bug in the new Makefile target? This is the reason the guidelines state that that globs should not conceal the soname.
> - When creating a ticket for tracking ExcludeArch, there isn't a component yet for qatlib. Is this created when the repo gets created or there is something I should do? Correct, it doesn't get created until you request the package source repo with `fedpkg request-repo`. Quoting from the guidelines I linked previously: > New packages will not have bugzilla entries during the review process, so they should put this description in the comment until the package is approved, then file the bugzilla entry, and replace the long explanation with the bug number.
> I tried a build reverting that myself, and something else looks off. Previously the build was creating the library files with this suffix: > > .so.%{soversion}.%{version} > > Now using the Makefile target it creates them with this suffix: > > .so.%{version} > > Is that a bug in the new Makefile target? This is the reason the guidelines state that that globs should not conceal the soname. That is expected. The upstream library is now installing .so.%{soversion} .so.%{version} Before, instead, the upstream library was not using the correct version in the real name. So we had .so.%{soversion} .so.0.0.0 In the spec we were renaming .so.0.0.0 into .so.%{soversion}.%{version} and then creating symlinks to .so.%{soversion} In the new version of the spec I removed the glob in the %files section and re-introduced a %global soversion with the difference that now there is a soversion for each library we install since both might evolve independently. Is this ok? Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib-20.10.0-1.fc33.src.rpm Regarding the git repo, I put a request for qatlib: https://pagure.io/releng/fedora-scm-requests/issue/30688
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/qatlib
> Correct, it doesn't get created until you request the package source repo with `fedpkg request-repo`. The repo is now created, however the qatlib component is not visible yet in bugzilla.
> In the spec we were renaming .so.0.0.0 into .so.%{soversion}.%{version} and then creating symlinks to .so.%{soversion} I'm not an expert in C libraries, but in a situation like this I'd prefer to follow the pattern of other Fedora libraries. Here are a few examples. curl-7.73.0-2.fc34: libcurl.so -> libcurl.so.4.7.0 libcurl.so.4 -> libcurl.so.4.7.0 libcurl.so.4.7.0 yelp-3.38.1-1.fc34: libyelp.so -> libyelp.so.0.0.0 libyelp.so.0 -> libyelp.so.0.0.0 libyelp.so.0.0.0 rpm-libs-4.16.0-3.fc34: librpm.so -> librpm.so.9.1.0 librpm.so.9 -> librpm.so.9.1.0 librpm.so.9.1.0 libxcb-1.13.1-6.fc34: libxcb.so -> libxcb.so.1.1.0 libxcb.so.1 -> libxcb.so.1.1.0 libxcb.so.1.1.0 You can see that the main library file with three digits is used as-is (not renamed), with symlinks for the other names. They do not include the software version in the library file name. Why does qatlib need to rename the library file to include the software version? I think the following file layout is what we should go with. Can the Makefile be adjusted to produce this? libqat.so -> libqat.so.0.0.0 libqat.so.0 -> libqat.so.0.0.0 libqat.so.0.0.0 libusdm.so -> libusdm.so.0.0.0 libusdm.so.0 -> libusdm.so.0.0.0 libusdm.so.0.0.0 It's ok to glob these files as long as it doesn't conceal the major version, so this is what I'd put in the %files section. %{_libdir}/libqat.so.%{libqat_soversion}* %{_libdir}/libusdm.so.%{libusdm_soversion}* > The repo is now created, however the qatlib component is not visible yet in bugzilla. I'm not sure if this is just a delay, or if it needs some other event first such as the first build in koji. Check back on this task after the first build is done.
> Why does qatlib need to rename the library file to include the software version? The main reason why this was done in the upstream project was to have a simple way to understand which version of the library is installed in the system by looking at /lib64/. This is mainly an issue when the library is built and installed from sources but is not when the RPM is installed since you can check what rpm is installed. The libraries were renamed as: libqat.so -> libqat.so.20.10.0 libqat.so.0 -> libqat.so.20.10.0 libqat.so.20.10.0 The makefile can be changed as you proposed to have libqat.so -> libqat.so.0.0.0 libqat.so.0 -> libqat.so.0.0.0 libqat.so.0.0.0 as this is the default produced by autotools. In order to be able to detect the version we can add an additional symlink that includes in the name the version of the library: libqat-20.10.so -> libqat.so.0.0.0 If this option is preferred we need to go for an additional upstream release (I have a patch already for that). It might take a couple of days to get it approved due to the internal process. BTW, I also checked other libraries and it seems that there isn't consistency. For e.g. this is the approach used by libc: libc.so.6 -> libc-2.32.so libc-2.32.so libc.so In any case, is the realname of the library a problem? In the current schema the realname updates at every release and it is independent from the fully qualified soname (which includes the soversion). Applications will be using the fully qualified soname and not referring directly to the realname. Regarding the component, I see now that qatlib has been added to Bugzilla. I can now create a ticket for tracking the ExcludeArch.
The upstream project has been updated so that the libraries produced are: libqat.so -> libqat.so.0.0.0 libqat.so.0 -> libqat.so.0.0.0 libqat.so.0.0.0 I also open a ticket for the Exclude arch and updated the spec: https://bugzilla.redhat.com/show_bug.cgi?id=1897661 The updated spec file can be found here: Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib-20.10.0-1.fc33.src.rpm If you are ok with this, I will upload the spec to dist-git.
> BTW, I also checked other libraries and it seems that there isn't consistency. For e.g. this is the approach used by libc: > libc.so.6 -> libc-2.32.so > libc-2.32.so > libc.so The majority of libraries follow the pattern I described. I didn't claim 100% consistency. :) > In any case, is the realname of the library a problem? > In the current schema the realname updates at every release and it is independent from the fully qualified soname (which includes the soversion). > Applications will be using the fully qualified soname and not referring directly to the realname. It's a problem if it can be confused with the soversion. And since applications will reference the soversion, it's unnecessary to include the software version in the library filename at all. > In order to be able to detect the version we can add an additional symlink that includes in the name the version of the library: > libqat-20.10.so -> libqat.so.0.0.0 I'm ok with a symlink like this because it avoids confusing the software version with the soversion, similar to how libc is named. Make sure to add those symlinks to the %files section. %{_libdir}/libqat-%{version}.so %{_libdir}/libusdm-%{version}.so > If this option is preferred we need to go for an additional upstream release (I have a patch already for that). It might take a couple of days to get it approved due to the internal process. Instead of waiting for the new upstream release, you can just include the patch in the spec file (Patch0: <name>.patch) and apply it during %prep. You're already using %autosetup, you'll just need to add a -p1 flag (assuming the strip offset of your patch is 1, which is pretty standard). Make sure to include either a comment explaining the patch or a link to an upstream issue/pull request/commit, as described in the patch guidelines [0]. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_patch_guidelines
In the end changed the upstream release to use the names you suggested as the change was small (just remove the logic in the makefile to rename the library). We didn't include a symlink containing the version in the name. This will be included starting from the next upstream release.
Is it ok if I push the src rpm to dist-git? Just to summarize: * The upstream release (20.10) was changed to use the default numbering scheme produced by autotools/libtool (as you suggested) - see comment #24 * The spec file was updated to explicitly use %{libname_so_version} in the %files section * A bugzilla ticket was opened to track the ExcludeArch and a reference to it was added to the spec The updated spec file can be found here if you want to have a look: Spec URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib.spec SRPM URL: https://raw.githubusercontent.com/intel/qatlib/v20_10/rpm/qatlib-20.10.0-1.fc33.src.rpm Build on copr: https://copr.fedorainfracloud.org/coprs/gcabiddu/qatlib/build/1772225/ Build on koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=55751536
Thanks for making those changes upstream. Yes, the current SRPM is ready to be imported into dist-git. One final piece of advice, it's a bad idea to force push and move tags in the upstream repo. It breaks a lot of expectations and some tools when a tarball is different if fetched at different times. Tagging a new release (such as 20.10.1) or applying patch files with the spec file is the recommended approach.
Thanks Carl. I imported the SRPM into dist-git.