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
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.
Copr build: https://copr.fedorainfracloud.org/coprs/build/6680913 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251086-rocfft/fedora-rawhide-x86_64/06680913-rocfft/fedora-review/review.txt 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.
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?
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
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 \
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.
[fedora-review-service-build]
Created attachment 2002809 [details] The .spec file difference from Copr build 6680913 to 6725634
Copr build: https://copr.fedorainfracloud.org/coprs/build/6725634 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251086-rocfft/fedora-rawhide-x86_64/06725634-rocfft/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
(In reply to 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?
(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?
(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.
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/
(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.
(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.
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/
Source url format follows https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags [fedora-review-service-build]
Copr build: https://copr.fedorainfracloud.org/coprs/build/6732156 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251086-rocfft/fedora-rawhide-x86_64/06732156-rocfft/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
LGTM.
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/fedora-rawhide-x86_64/06867009-rocfft/fedora-review/review.txt
Created attachment 2007419 [details] The .spec file difference from Copr build 6732156 to 6867163
Copr build: https://copr.fedorainfracloud.org/coprs/build/6867163 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251086-rocfft/fedora-rawhide-x86_64/06867163-rocfft/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
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.
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}/
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#/rocFFT-rocm-6.0.0.tar.gz : 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/%{upstreamname} 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
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.
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
(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.
Ok, that is fine to have testing package.
(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/%{upstreamname} > 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/
Created attachment 2008944 [details] The .spec file difference from Copr build 6867163 to 6910837
Copr build: https://copr.fedorainfracloud.org/coprs/build/6910837 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251086-rocfft/fedora-rawhide-x86_64/06910837-rocfft/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Thanks. Approved.
The Pagure repository was created at https://src.fedoraproject.org/rpms/rocfft
rocfft-6.0.0-2 built in koji - https://koji.fedoraproject.org/koji/buildinfo?buildID=2346371