Spec URL: https://trix.fedorapeople.org/rocprim.spec SRPM URL: https://trix.fedorapeople.org/rocprim-5.5.1-1.fc39.src.rpm FAS: trix The rocPRIM is a header-only library providing HIP parallel primitives for developing performant GPU-accelerated code on AMD ROCm platform. Reproducible: Always
I have a bit of a list of concerns before I run through the review. head-only libraries are inherently static, so you need to add the %{name}-static provides to the devel package: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_header_only_libraries rpmlint will likely complain that you only put only noarch files in libdir. I'd recommend just adding -DCMAKE_INSTALL_LIBDIR=share to install the cmake into datadir. This is actually what debian does: https://salsa.debian.org/rocm-team/rocprim/-/blob/master/debian/rules#L13 As well, I touched base with the upstream developers and the intention is to install it in datadir in the long term, but there's a bug in rocm-cmake. I wonder if the devel package should be made noarch... you would need to peek into the cmake files to see if it injects any compiler flags. If it doesn't I would recommend making the devel package noarch but keeping the top level normal due to the build requirements. These lines should be removed: > -DCMAKE_INSTALL_INCLUDEDIR=%{_includedir} \ > %exclude /usr/rocprim And you should add "-DROCM_SYMLINK_LIBS=OFF" like I did with rocrand. This should fix the odd installation. My gut says that you should add NOTICES.txt to the license macro too, since it contains copyright information. I would add "BuildRequires: doxygen" so you don't forget to package the doxygen files when they fix the cmake to build it by default. So the two optflags that hip libraries don't like are "-fstack-protector-strong" and "-fcf-protection". I tested and if you prepend "-Xarch_host " it will resolve the issue and allow enabling hardening. Note that Debian does this with fstack-protector-strong. E.g. something crude like this works: > %global optflags %(echo %{optflags} | sed -e 's/-fstack-protector-strong/-Xarch_host -fstack-protector-strong/' -e 's/-fcf-protection/-Xarch_host -fcf-protection/' | sed 's/-Xarch_host -Xarch_host/-Xarch_host/g') I'm thinking I should patch this logic into hipcc to save us some future headaches, as it would allow hip libraries to work without touching the optflags or disabling hardening. Any thoughts?
I will post the requested changes shortly. On the global optflags hackery.. I do not think hipcc should be were the change is made as you do not want to chase every incorrect flag or ignoring them by default. Could infra for hipc be added as a %toolchain ? Maybe Tom knows ?
Good point, we should have a toolchain option for hipcc if that makes the most sense. As far as I know, -Xarch_host should allow setting the flag for host code, but not for the HIP targeted code where it's not supported, so we can still have hardening where applicable. Eitherway, we should develop a standard for all the ROCm HIP libraries to follow and then document it.
Spec URL: https://trix.fedorapeople.org/rocprim.spec SRPM URL: https://trix.fedorapeople.org/rocprim-5.5.1-2.fc39.src.rpm This is a noarch package, but I went with an exclusive arch because to be useful the rest of rocm needs to be supported on more than x86_64. We would have unhappy ppc, arm and s390 folks that installed this package and got frustrated that nothing else worked.
> This is a noarch package, but I went with an exclusive arch because to be useful the rest of rocm needs to be supported on more than x86_64. We would have unhappy ppc, arm and s390 folks that installed this package and got frustrated that nothing else worked. Well you can set both exclusive arch AND noarch and it will work (they aren't mutually exclusive). See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_noarch_with_unported_dependencies But if you use noarch, I think you should only apply noarch to the devel subpackage to tell koji to specifically build it on a x86_64 machine only. I'm not 100% sure if this is correct. E.g: %package devel BuildArch: noarch But I'm also okay with keeping as-is if you think that's clearer/cleaner. I kicked off a copr build, and will review it when it's done: https://copr.fedorainfracloud.org/coprs/mystro256/rocm-hip/build/6087516/
> But if you use noarch, I think you should only apply noarch to the devel subpackage to tell koji to specifically build it on a x86_64 machine only. I'm not 100% sure if this is correct. I re-read the guidelines page. I think you can just set noarch and exclusive arch in the same place and this should work. There's no need to make devel package specifically noarch if I understand correctly.
There's still a few issues, I added inline comments. I strongly think you should add "noarch" if you think the package is truly noarch, like so: +BuildArch: noarch # ROCm only working on x86_64 ExclusiveArch: x86_64 IIUC, it should mean that it's noarch but it only should be built on and supplied with x86_64. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Note: No gcc, gcc-c++ or clang found in BuildRequires See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ > Looks like a fail positive. You added clang-devel, it's fine ===== MUST items ===== C/C++: [X]: Package does not contain kernel modules. [X]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [-]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages [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", "MIT License", "BSD 3-Clause License", "MIT License [generated file]". 51 files have unknown license. Detailed output of licensecheck in /var/lib/copr- rpmbuild/results/rocprim/licensecheck.txt > I believe you should update the license field to "MIT and BSD 3-Clause License" [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/share/doc/rocprim [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/doc/rocprim > You need to own this directory, either add %{docdir}/rocprim to %files or you can do what I did in rocrand. > For ROCm packages I usually just exclude this and in the %license macro I specify the file I want. > Up to you how you implement it. [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. [x]: 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. [-]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 3 files. [x]: Package complies to the Packaging Guidelines [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. [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 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]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [-]: 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). [?]: 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. [-]: Package should compile and build into binary rpms on all supported architectures. > It doesn't but I think it's justified [?]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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: [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched.
Spec URL: https://trix.fedorapeople.org/rocprim.spec SRPM URL: https://trix.fedorapeople.org/rocprim-5.5.1-3.fc39.src.rpm See above for the updates.
You could probably use: > %{_docdir}/%{name} > %{_datadir}/cmake/%{name} For consistency. I tested building rocsparse and it builds fine, but since it's a mix of C++ and Fortran, it made me realise that we should probably not muck around with global optflags, but rather just only change c++, like so: > %global build_cxxflags %(echo %{optflags} | sed -e 's/-fstack-protector-strong/-Xarch_host -fstack-protector-strong/' -e 's/-fcf-protection/-Xarch_host -fcf-protection/') It also simplifies the sed hackery. I'll been patching rocrand shortly with this. Anyway, not huge blockers. Approved.
Also: -License: MIT and BSD 3-Clause License +License: MIT and BSD Or rpmlint will complain
The Pagure repository was created at https://src.fedoraproject.org/rpms/rocprim
The needinfo request[s] on this closed bug have been removed as they have been unresolved for 120 days