https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Bug ID: 2251086 Summary: Review Request: rocfft - ROCm Fast Fourier Transforms (FFT) library Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: tflink@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://tflink.fedorapeople.org/packages/rocfft/rocfft.spec SRPM URL: https://tflink.fedorapeople.org/packages/rocfft/rocfft-5.7.1-1.fc40.src.rpm COPR Build: https://copr.fedorainfracloud.org/coprs/tflink/rocm-packaging/build/6680904/ Description: A library for computing Fast Fourier Transforms (FFT), part of ROCm. Fedora Account System Username: tflink
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #1 from Tim Flink tflink@redhat.com --- Since I'm sure this will come up: I intentionally set this up such that the -test subpackage is built. I don't see a reasonable way to build just the tests against a binary package so this is the only way to run the upstream test suite against rpms built in koji.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/ROCmSoft | |warePlatform
--- Comment #2 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6680913 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review? Assignee|nobody@fedoraproject.org |benson_muite@emailplus.org Status|NEW |ASSIGNED CC| |benson_muite@emailplus.org
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #3 from Benson Muite benson_muite@emailplus.org --- a) Warnings from fedora-review
rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rocfft-test rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rtc_helper_crash rocfft.src: E: description-line-too-long rocFFT is a software library for computing Fast Fourier Transforms (FFT) written in HIP. It is part of AMD's software ecosystem based on ROCm. rocfft.x86_64: E: description-line-too-long rocFFT is a software library for computing Fast Fourier Transforms (FFT) written in HIP. It is part of AMD's software ecosystem based on ROCm.
b) Can tests and/or samples be run as a check of correct functionality?
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #4 from Benson Muite benson_muite@emailplus.org --- c) Please replace Source0: %{url}/%{upstreamname}/archive/refs/tags/rocm-%{version}.tar.gz#/%{upstreamname}-rocm-%{version}.tar.gz with Source0: %{url}/%{upstreamname}/archive/rocm-%{version}.tar.gz#/%{upstreamname}-rocm-%{version}.tar.gz
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #5 from Tom Rix trix@redhat.com --- some changes to use the rocm toolchaindiff --git a/rocfft.spec b/rocfft.spec index 64ade62..7956e76 100644 --- a/rocfft.spec +++ b/rocfft.spec @@ -7,11 +7,9 @@ # rocFFT has a version seperate from the ROCm version that it is released with %global rocfft_version 1.0.23
-# Compiler is hipcc, which is clang based: -%global toolchain clang - -#The target doesn't support -fcf-protection: -%global optflags %(o="%{optflags}"; echo ${o//-fcf-protection/}) +%global toolchain rocm +# hipcc does not support some clang flags +%global build_cxxflags %(echo %{optflags} | sed -e 's/-fstack-protector-strong/-Xarch_host -fstack-protector-strong/' -e 's/-fcf-protection/-Xarch_host -fcf-protection/')
# the debug build has a completely different file name, use condition to enable/disable %bcond rocm-debug 0 @@ -53,6 +51,7 @@ BuildRequires: llvm-devel BuildRequires: rocm-cmake BuildRequires: rocm-comgr-devel BuildRequires: rocm-hip-devel +BuildRequires: rocm-rpm-macros BuildRequires: rocm-runtime-devel BuildRequires: sqlite-devel
@@ -107,15 +106,13 @@ self tests for the rocfft library %if %{with rocm-debug} -DCMAKE_BUILD_TYPE=Debug \ %else - -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo \ %endif %if %{with kernelcache} -DROCFFT_KERNEL_CACHE_ENABLE=ON \ %else -DROCFFT_KERNEL_CACHE_ENABLE=OFF \ %endif - -DCMAKE_CXX_COMPILER=hipcc \ - -DCMAKE_C_COMPILER=hipcc \
-DAMDGPU_TARGETS="gfx803;gfx900;gfx906;gfx908;gfx90a;gfx1030;gfx1100;gfx1101;gfx1102" \ -DBUILD_FILE_REORG_BACKWARD_COMPATIBILITY=OFF \ -DSQLITE_USE_SYSTEM_PACKAGE=ON \
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #6 from Tom Rix trix@redhat.com --- For Ben's question about %check Checking depends on having an AMD GPU in the build machine, that is not possible. The -test subpackage is consistent with how other rocm packages have handled this. However in the other packages -test is off by default and not expected to be part of the offical fedora release. We should be consistent, can rocfft also have -test off by default ?
New comments CMAKE_BUILD_TYPE=Debug We have not figured out how or if to do this. I know from my own experiments that this will cause packing errors when the rpm tooling does not handle some clang-ism. And if we did have a -debug, I would not want to *-d.so lib names.
Above, I also changed the RELEASE to RelWithDebInfo so it would be consistent with other packages.
The explicit list of AMDGPU_TARGETS is missing some, you can see in the cmake output that gfx94x is checked. If we need a full list, I think it would be useful to add that to rocm-rpm-macros package macros.rocm file.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #7 from Benson Muite benson_muite@emailplus.org --- [fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #8 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2002809 --> https://bugzilla.redhat.com/attachment.cgi?id=2002809&action=edit The .spec file difference from Copr build 6680913 to 6725634
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #9 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6725634 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #10 from Tim Flink tflink@redhat.com --- (In reply to Benson Muite from comment #4)
c) Please replace Source0: %{url}/%{upstreamname}/archive/refs/tags/rocm-%{version}.tar.gz#/ %{upstreamname}-rocm-%{version}.tar.gz with Source0: %{url}/%{upstreamname}/archive/rocm-%{version}.tar.gz#/%{upstreamname}-rocm- %{version}.tar.gz
I'm not really arguing but I'm curious: why change the archive link?
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #11 from Tim Flink tflink@redhat.com --- (In reply to Tom Rix from comment #6)
For Ben's question about %check Checking depends on having an AMD GPU in the build machine, that is not possible. The -test subpackage is consistent with how other rocm packages have handled this. However in the other packages -test is off by default and not expected to be part of the offical fedora release. We should be consistent, can rocfft also have -test off by default ?
I probably should have started a conversation about this before submitting this for review, but I actually want to change all the rocm packages to build the -test subpackages by default unless the behavior of rocfft is significantly different from the other rocm packages. There's no other reasonable way I can see to test the binaries shipped with Fedora - cmake will detect the lack of a build to test against if you build the tests alone and build from source rather than use an installed binary. You can use the rpm libraries for testing but building the entire library for every test seems like an incredible waste, especially if we end up enabling the various cache features at any point. The build time for rocfft with kernel db enabled is over an hour on my machine IIRC.
New comments CMAKE_BUILD_TYPE=Debug We have not figured out how or if to do this. I know from my own experiments that this will cause packing errors when the rpm tooling does not handle some clang-ism. And if we did have a -debug, I would not want to *-d.so lib names.
Above, I also changed the RELEASE to RelWithDebInfo so it would be consistent with other packages.
Thanks, I didn't realize that was an option and have already had a bit of fun with the changing library names when debug was enabled.
The explicit list of AMDGPU_TARGETS is missing some, you can see in the cmake output that gfx94x is checked. If we need a full list, I think it would be useful to add that to rocm-rpm-macros package macros.rocm file.
I'm unclear on why we want to do this. I know that there are some rocm packages where the full library is too large when all of the gpu targets are enabled but rocfft isn't one of those. I get disabling some older targets to make the library size reasonable but why disable older card families if we don't have to?
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #12 from Tim Flink tflink@redhat.com --- (In reply to Tim Flink from comment #11)
The explicit list of AMDGPU_TARGETS is missing some, you can see in the cmake output that gfx94x is checked. If we need a full list, I think it would be useful to add that to rocm-rpm-macros package macros.rocm file.
I'm unclear on why we want to do this. I know that there are some rocm packages where the full library is too large when all of the gpu targets are enabled but rocfft isn't one of those. I get disabling some older targets to make the library size reasonable but why disable older card families if we don't have to?
NVM, I managed to misread what you wrote. Ignore that comment, please.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #13 from Tim Flink tflink@redhat.com --- I've updated the spec file to address comments and made a new copr build. F39 build fails due to no rocm-rpm-macros.
spec file: https://tflink.fedorapeople.org/packages/rocfft/rocfft.spec new build: https://copr.fedorainfracloud.org/coprs/tflink/rocm-packaging/build/6727699/
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #14 from Tom Rix trix@redhat.com --- (In reply to Tim Flink from comment #12)
(In reply to Tim Flink from comment #11)
The explicit list of AMDGPU_TARGETS is missing some, you can see in the cmake output that gfx94x is checked. If we need a full list, I think it would be useful to add that to rocm-rpm-macros package macros.rocm file.
I'm unclear on why we want to do this. I know that there are some rocm packages where the full library is too large when all of the gpu targets are enabled but rocfft isn't one of those. I get disabling some older targets to make the library size reasonable but why disable older card families if we don't have to?
NVM, I managed to misread what you wrote. Ignore that comment, please.
By using an explicit list, you are missing some gpu's. I am asking why these gpus are missing.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #15 from Tim Flink tflink@redhat.com --- (In reply to Tom Rix from comment #14)
(In reply to Tim Flink from comment #12)
(In reply to Tim Flink from comment #11)
The explicit list of AMDGPU_TARGETS is missing some, you can see in the cmake output that gfx94x is checked. If we need a full list, I think it would be useful to add that to rocm-rpm-macros package macros.rocm file.
I'm unclear on why we want to do this. I know that there are some rocm packages where the full library is too large when all of the gpu targets are enabled but rocfft isn't one of those. I get disabling some older targets to make the library size reasonable but why disable older card families if we don't have to?
NVM, I managed to misread what you wrote. Ignore that comment, please.
By using an explicit list, you are missing some gpu's. I am asking why these gpus are missing.
Unfortunately, I started packaging rocfft so long ago, I don't remember. It doesn't match the default from cmake so I must have copied it from somewhere.
I'll remove it as I can't think of a reason to limit the gpu families for rocfft, it doesn't have size issues with expanding that list. Thanks for catching it.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #16 from Tim Flink tflink@redhat.com --- Updated spec to remove GPU family list
spec file: https://tflink.fedorapeople.org/packages/rocfft/rocfft.spec new build: https://copr.fedorainfracloud.org/coprs/tflink/rocm-packaging/build/6729583/
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #17 from Benson Muite benson_muite@emailplus.org --- Source url format follows https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_ta...
[fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #18 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6732156 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #19 from Tom Rix trix@redhat.com --- LGTM.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #20 from Tim Flink tflink@redhat.com --- I've updated rocfft for the 6.0 release of ROCm.
spec file: https://tflink.fedorapeople.org/packages/rocfft/rocfft.spec srpm: https://tflink.fedorapeople.org/packages/rocfft/rocfft-6.0.0-1.fc40.src.rpm copr build: https://copr.fedorainfracloud.org/coprs/tflink/rocm-packaging/build/6867009/ review.txt: https://download.copr.fedorainfracloud.org/results/tflink/rocm-packaging/fed...
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #21 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2007419 --> https://bugzilla.redhat.com/attachment.cgi?id=2007419&action=edit The .spec file difference from Copr build 6732156 to 6867163
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |AutomationTriaged
--- Comment #22 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6867163 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #23 from Tom Rix trix@redhat.com --- rocfft.spec: E: specfile-error sed: -e expression #2, char 44: unterminated `s' command %global build_cxxflags %(echo %{optflags} | sed -e 's/-fstack-protector-strong/-Xarch_host -fstack-protector-strong/' -e 's/-fcf-protection/-Xarch_host-fcf-protection')
I think you are missing the last '/'
rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rocfft-test rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rtc_helper_crash If -test was an optional package this would be fine, but if you want to distribute the -test, then these need to be -fPIC-ed. My non blocking preference is for an optional package.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2257069
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2257069 [Bug 2257069] Review Request: hipfft - ROCm FFT marshalling library
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #24 from Tom Rix trix@redhat.com --- For hipFFT, some headers need to be copied . https://bugzilla.redhat.com/show_bug.cgi?id=2257069
This line or similar needs to be added to the %install
# Needed by hipFFT # https://github.com/ROCm/hipFFT/issues/83 cp -r %{_vpath_srcdir}/shared %{buildroot}/%{_includedir}/%{name}/
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #25 from Benson Muite benson_muite@emailplus.org --- Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
===== MUST items =====
C/C++: [ ]: Provides: bundled(gnulib) in place as required. Note: Sources not installed [ ]: Package does not contain kernel modules. [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. [x]: Development (unversioned) .so files in -devel subpackage, if present.
Generic: [ ]: 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", "MIT License". 76 files have unknown license. Detailed output of licensecheck in /home/fedora/2251086-rocfft/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [ ]: %build honors applicable compiler flags or justifies otherwise. [ ]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [ ]: Sources contain only permissible code or content. [x]: 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. [ ]: Package does not generate any conflict. [ ]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [ ]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [ ]: Useful -debuginfo package or justification otherwise. [ ]: Package is not known to require an ExcludeArch tag. [ ]: 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 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]: 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 3703 bytes in 1 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). [ ]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [!]: %check is present and all tests pass. [ ]: 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]: Package should compile and build into binary rpms on all supported architectures. [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]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: rocfft-6.0.0-1.fc40.x86_64.rpm rocfft-devel-6.0.0-1.fc40.x86_64.rpm rocfft-test-6.0.0-1.fc40.x86_64.rpm rocfft-debuginfo-6.0.0-1.fc40.x86_64.rpm rocfft-debugsource-6.0.0-1.fc40.x86_64.rpm rocfft-6.0.0-1.fc40.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/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/tmp_xin4ora')] checks: 32, packages: 6
rocfft.spec: E: specfile-error sed: -e expression #2, char 44: unterminated `s' command rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rocfft-test rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rtc_helper_crash rocfft-test.x86_64: W: no-manual-page-for-binary rocfft-test rocfft-test.x86_64: W: no-manual-page-for-binary rtc_helper_crash rocfft-devel.x86_64: W: no-documentation rocfft-test.x86_64: W: no-documentation 6 packages and 0 specfiles checked; 1 errors, 6 warnings, 38 filtered, 1 badness; has taken 2.0 s
Rpmlint (debuginfo) ------------------- Checking: rocfft-debuginfo-6.0.0-1.fc40.x86_64.rpm rocfft-test-debuginfo-6.0.0-1.fc40.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/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/tmp88y4zbyh')] checks: 32, packages: 2
2 packages and 0 specfiles checked; 0 errors, 0 warnings, 22 filtered, 0 badness; has taken 1.0 s
Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/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 checks: 32, packages: 6
rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rocfft-test rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rtc_helper_crash rocfft-test.x86_64: W: no-manual-page-for-binary rocfft-test rocfft-test.x86_64: W: no-manual-page-for-binary rtc_helper_crash rocfft-devel.x86_64: W: no-documentation rocfft-test.x86_64: W: no-documentation 6 packages and 0 specfiles checked; 0 errors, 6 warnings, 47 filtered, 0 badness; has taken 1.8 s
Source checksums ---------------- https://github.com/ROCmSoftwarePlatform/rocFFT/archive/rocm-6.0.0.tar.gz#/ro... : CHECKSUM(SHA256) this package : fb8ba56572702e77e4383d922cd1fee4ad3fa5f63a5ebdb3d9c354439a446992 CHECKSUM(SHA256) upstream package : fb8ba56572702e77e4383d922cd1fee4ad3fa5f63a5ebdb3d9c354439a446992
Requires -------- rocfft (rpmlib, GLIBC filtered): ld-linux-x86-64.so.2()(64bit) libamdhip64.so.6()(64bit) libamdhip64.so.6(hip_4.2)(64bit) libamdhip64.so.6(hip_6.0)(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libsqlite3.so.0()(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.2)(64bit) libstdc++.so.6(CXXABI_1.3.3)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) libstdc++.so.6(CXXABI_1.3.7)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) rtld(GNU_HASH)
rocfft-devel (rpmlib, GLIBC filtered): cmake-filesystem(x86-64) librocfft.so.0()(64bit) rocfft(x86-64)
rocfft-test (rpmlib, GLIBC filtered): libamdhip64.so.6()(64bit) libamdhip64.so.6(hip_4.2)(64bit) libamdhip64.so.6(hip_4.3)(64bit) libamdhip64.so.6(hip_6.0)(64bit) libboost_program_options.so.1.81.0()(64bit) libc.so.6()(64bit) libfftw3.so.3()(64bit) libfftw3_omp.so.3()(64bit) libfftw3f.so.3()(64bit) libfftw3f_omp.so.3()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgtest.so.1.14.0()(64bit) libm.so.6()(64bit) libomp.so()(64bit) libomp.so(VERSION)(64bit) librocfft.so.0()(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.2)(64bit) libstdc++.so.6(CXXABI_1.3.3)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) rocfft(x86-64) rtld(GNU_HASH)
rocfft-debuginfo (rpmlib, GLIBC filtered):
rocfft-debugsource (rpmlib, GLIBC filtered):
Provides -------- rocfft: librocfft.so.0()(64bit) rocfft rocfft(x86-64)
rocfft-devel: cmake(rocfft) rocfft-devel rocfft-devel(x86-64)
rocfft-test: rocfft-test rocfft-test(x86-64)
rocfft-debuginfo: debuginfo(build-id) librocfft.so.0.1-6.0.0-1.fc40.x86_64.debug()(64bit) rocfft-debuginfo rocfft-debuginfo(x86-64)
rocfft-debugsource: rocfft-debugsource rocfft-debugsource(x86-64)
Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -b 2251086 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Shell-api, Generic Disabled plugins: Ocaml, Perl, Python, PHP, Haskell, fonts, SugarActivity, R, Java Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH
Comments: a) For the url field consider using either: https://github.com/ROCm/rocFFT or https://github.com/ROCm/%%7Bupstreamname%7D The repository https://github.com/ROCmSoftwarePlatform/rocFFT does not exist. b) The file CMakeLists.txt contains set ( VERSION_STRING "1.0.23" ), but website indicates version 1.0.25 - which is correct? Raised issue upstream https://github.com/ROCm/rocFFT/issues/453 c) Using %exclude %{_docdir}/%{name}/LICENSE.md is discouraged It is better to remove the extra installed copy of the license file after the install step like is done for client_info file and helper_binary d) There is a possibility to build samples and benchmarks, could some of these be used as smoke tests? e) Can your patches for GNUInstallDirs and rpath removal be upstreamed? Having them as options may be useful elsewhere and will make maintenance easier. f) Is it possible to add -pie flags to remove the warnings: rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rocfft-test rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rtc_helper_crash g) Can the documentation be built? Ideally as man pages. Some additional dependencies such as python3dist(sphinx) are needed as is packaging of https://github.com/RadeonOpenCompute/rocm-docs-core. Initially may want to just package the files in docs/samples
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #26 from Benson Muite benson_muite@emailplus.org --- For the CMake configuration, may want to add: -DFETCH_CONTENT_FULLY_DISCONNECTED=ON \ -DFETCH_CONTENT_QUEIT=ON \ -DROCFFT_BUILD_OFFLINE_TUNER=ON \ -DBUILD_CLIENTS_BENCH=ON \ -DBUILD_CLIENTS_SAMPLES=ON
To run tests probably: https://github.com/ROCm-Developer-Tools/HIP-CPU needs to be packaged.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #27 from Benson Muite benson_muite@emailplus.org --- Sorry, no need to build client samples or benchmarks at present. Would suggest just use: -DFETCH_CONTENT_FULLY_DISCONNECTED=ON \ -DFETCH_CONTENT_QUIET=ON \ -DROCFFT_BUILD_OFFLINE_TUNER=ON \
Please add a patch to bump the rocFFT version see: https://github.com/ROCm/rocFFT/pull/450
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #28 from Tom Rix trix@redhat.com --- (In reply to Benson Muite from comment #26)
For the CMake configuration, may want to add: -DFETCH_CONTENT_FULLY_DISCONNECTED=ON \ -DFETCH_CONTENT_QUEIT=ON \ -DROCFFT_BUILD_OFFLINE_TUNER=ON \ -DBUILD_CLIENTS_BENCH=ON \ -DBUILD_CLIENTS_SAMPLES=ON
To run tests probably: https://github.com/ROCm-Developer-Tools/HIP-CPU needs to be packaged.
There are no releases for this project, it is self described as "Please note the library is being actively developed, and is known to be incomplet; it might also be incorrekt and there could be a few bad bugs lurking" If we are to depend on something to check the other packages, it needs to be stable and aligned with the other rocm packages. Otherwise we will spend a lot of time chasing false problems. So I would rather we did not package it.
Testing depends on a gpu. The testing I have set in the other rocm packages is to build an optional -test subpackage so the tests can be installed to a machine that has a gpu. My preference is for rocFFT to do similar.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #29 from Benson Muite benson_muite@emailplus.org --- Ok, that is fine to have testing package.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #30 from Tim Flink tflink@redhat.com --- (In reply to Benson Muite from comment #25) <snip>
Comments: a) For the url field consider using either: https://github.com/ROCm/rocFFT or https://github.com/ROCm/%%7Bupstreamname%7D The repository https://github.com/ROCmSoftwarePlatform/rocFFT does not exist.
I knew that the upstream project was going to change but didn't realize it had already happen. thanks for catching that
b) The file CMakeLists.txt contains set ( VERSION_STRING "1.0.23" ), but website indicates version 1.0.25 - which is correct? Raised issue upstream https://github.com/ROCm/rocFFT/issues/453
patched until that's fixed upstream
c) Using %exclude %{_docdir}/%{name}/LICENSE.md is discouraged It is better to remove the extra installed copy of the license file after the install step like is done for client_info file and helper_binary
fixed
d) There is a possibility to build samples and benchmarks, could some of these be used as smoke tests?
I added the samples to the dev subpackage. benchmarks have been discussed in other comments
e) Can your patches for GNUInstallDirs and rpath removal be upstreamed? Having them as options may be useful elsewhere and will make maintenance easier.
I opted not to submit that one upstream as it's somewhat of a fedora-ism where not all consumers of the rocm components use GNU install dirs.
f) Is it possible to add -pie flags to remove the warnings: rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rocfft-test rocfft-test.x86_64: W: position-independent-executable-suggested /usr/bin/rtc_helper_crash
Done
g) Can the documentation be built? Ideally as man pages. Some additional dependencies such as python3dist(sphinx) are needed as is packaging of https://github.com/RadeonOpenCompute/rocm-docs-core. Initially may want to just package the files in docs/samples
That will require a few leaf packages in addition to rocm-docs-core and the result is mostly what is already available upstream which seems like a bit of a duplication of effort. I agree that it'd be nice to have man pages but is it a deal breaker for the review?
(In reply to Benson Muite from comment #27)
Sorry, no need to build client samples or benchmarks at present. Would suggest just use: -DFETCH_CONTENT_FULLY_DISCONNECTED=ON \ -DFETCH_CONTENT_QUIET=ON \ -DROCFFT_BUILD_OFFLINE_TUNER=ON \
According to the build log, CMake says that FETCH_CONTENT_FULLY_DISCONNECTED and FETCH_CONTENT_QUIET are unused by this project. Do you still think they need to be added?
Spec URL: https://tflink.fedorapeople.org/packages/rocfft/rocfft.spec SRPM URL: https://tflink.fedorapeople.org/packages/rocfft/rocfft-6.0.0-2.fc40.src.rpm COPR Build: https://copr.fedorainfracloud.org/coprs/tflink/rocm-packaging/build/6910671/
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #31 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2008944 --> https://bugzilla.redhat.com/attachment.cgi?id=2008944&action=edit The .spec file difference from Copr build 6867163 to 6910837
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #32 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6910837 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+ Status|ASSIGNED |POST
--- Comment #33 from Benson Muite benson_muite@emailplus.org --- Thanks. Approved.
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
--- Comment #34 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/rocfft
https://bugzilla.redhat.com/show_bug.cgi?id=2251086
Tim Flink tflink@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |NEXTRELEASE Last Closed| |2024-01-17 21:17:15
--- Comment #35 from Tim Flink tflink@redhat.com --- rocfft-6.0.0-2 built in koji - https://koji.fedoraproject.org/koji/buildinfo?buildID=2346371
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com