Spec URL: https://trix.fedorapeople.org/rocm-rpm-macros.spec SRPM URL: https://trix.fedorapeople.org/rocm-rpm-macros-5.7.0-1.fc40.src.rpm The ROCm packages have similar boilerplate that gets cut and pasted. This package collects that to a common set of macros that they can all use cutting down on the boiler plate and making the packages consistent. Some features A rocm toolchain Common rocm_release Handing on the rocm modules An example of use in an existing package ishttps://src.fedoraproject.org/fork/trix/rpms/rocrand/c/b1ca7222dd8c8609cc0f08fdb9a36098002802f4?branch=rocm-macros-dev An example of use in a new package with modules is https://github.com/trixirt/rocBLAS/commit/deba70490ff6af3f866553fe703f4e00e6da3067 Reproducible: Always
Copr build: https://copr.fedorainfracloud.org/coprs/build/6508209 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242859-rocm-rpm-macros/fedora-rawhide-x86_64/06508209-rocm-rpm-macros/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.
You said you couldn't get build_cxxflags working right? If so, I suggest setting "rocm_cxxflags" for now and then we can add "%define build_cxxflags %rocm_cxxflags" until it's figured out. I'd like to abstract away all the quirkiness from the rocm spec files if possible. There's also a the CMAKE_CXX_FLAGS option that you could add to rocm_cmake_options if that helps.
in the interior macros, there is an assumption that these flags will be handled and the are for gcc and clang. So hipcc should handle them, the better fix would be to move this logic there.
OK, I'll draft a patch for hipcc and see if we can drop it from there. Otherwise the review looks good. I'll give it an official review once I take a look at the modules (or someone else takes it)
Some thoughts though: Why not merge macros and modules together? Since they use the same source, it seems more logical to me and reduces the update churn. I.e. make the module package a subpackage of this. I also suggest that you tag the source, for cleaner packaging and easier viewing on github. In hindsight, I'm not sure I'm 100% keen on doing the rocm_release/rocm_patch macros in macros.rocm, as it makes it less clear what release is being build in the rocm hip libs spec files. If you insist on them, I'd recommend generating the them in this spec file instead, e.g. in %prep or %build (you could make a simple makefile that use templates for example), to reduce code churn when we bump rocm version.
Originally they were together, I split them because the module are used outside of building, so why would we want rpm stuff when we were not doing building. So this is more about keeping the install clean. subpackaging is fine, which is the controlling one ? Having a central rocm_release means all of the packages could be updated at the same time without touching every package's spec file. if we do not want to do that, lets just remove them from the macros.rocm
> So this is more about keeping the install clean. subpackaging is fine, which is the controlling one ? Yes of course, they should be separate packages for sure. I was thinking have rocm-rpm-macros be the controling package, since it's the name you gave it in github. > Having a central rocm_release means all of the packages could be updated at the same time without touching every package's spec file. if we do not want to do that, lets just remove them from the macros.rocm My gut says it's little value added, as there will always be updates to the spec file due to fedora rebuilds. Recently I updated packages to 5.7 and had to flip all the release fields from 2 to 1. We could avoid churn by adding autorelease to all the spec files, but I think you still need to make a commit for it to work correctly. I also am a bit concerned that the macro might cause difficulties with reproducible builds. E.g/ if I explicitly wanted to rebuild rocblas from 5.6 against 5.7 to debug an issue, it might be less straightforward having the spec file depend on these macros.
I'm generally ok with it, I can do a formal review, but I request a few things if they are ok with you: - drop rocm_release, at least for now (I'm not convinced this will save us much effort and might cause grief) - please add the module package as a subpackage of this (single source to single spec file makes packaging and updating generally easier)
Spec URL: https://trix.fedorapeople.org/rocm-rpm-macros.spec SRPM URL: https://trix.fedorapeople.org/rocm-rpm-macros-1.0-2.fc40.src.rpm release dropped. This is how it changes blas --- a/rocblas.spec +++ b/rocblas.spec @@ -1,4 +1,7 @@ %global upstreamname rocBLAS +%global rocm_release 5.7 +%global rocm_patch 0 +%global rocm_version %{rocm_release}.%{rocm_patch} %global toolchain rocm # hipcc does not support some clang flags modules are a subpackage
Created attachment 1993651 [details] The .spec file difference from Copr build 6508209 to 6520829
Copr build: https://copr.fedorainfracloud.org/coprs/build/6520829 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242859-rocm-rpm-macros/fedora-rawhide-x86_64/06520829-rocm-rpm-macros/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.
Some suggestions (non-blocking): - If you want, you can compress the mkdir and cp command for the macros file into one command: install -Dpm 644 macros.rocm %{buildroot}%{_rpmmacrodir}/macros.rocm - You can create a tag in github to reduce the few commit related lines at the top of the spec; since you version the package as 1.0, it seems logical to me to tag 1.0 Otherwise, see inline comments. There's two issues that need resolving, but else wise it's good. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Dist tag is present. > You forgot to add the ? in %{?dist} - The License field must be a valid SPDX expression. Note: Not a valid SPDX expression 'GPLv2'. It seems that you are using the old Fedora license abbreviations. Try `license-fedora2spdx' for converting it to SPDX. See: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 > Please use GPL-2.0-or-later or GPL-2.0-only (I didn't see if you opted for the former or the latter). Maybe it accepts "GPL-2.0" as well, but I'm not sure. ===== MUST items ===== Generic: [x]: 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. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. No licenses found. Please check the source files for licenses manually. [x]: License file installed when any subpackage combination is installed. [?]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib/rpm, /usr/lib/rpm/macros.d > I believe it's complaining because you omitted a "Requires: rpm" on both packages. With that said, I highly doubt it's required because how else would you install the package :) [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. [-]: Development files must be in a -devel package [-]: 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]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Test run failed [x]: Package complies to the Packaging Guidelines [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 does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [!]: 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). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rocm- rpm-macros-modules [?]: 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. [x]: Package should compile and build into binary rpms on all supported architectures. [ ]: %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.
Spec URL: https://trix.fedorapeople.org/rocm-rpm-macros.spec SRPM URL: https://trix.fedorapeople.org/rocm-rpm-macros-1.0-3.fc40.src.rpm For the license and dist fixes. For tagging the repo, I was thinking the fedora repo would be a better location than my github, its only 5 files.
Created attachment 1993785 [details] The .spec file difference from Copr build 6520829 to 6524159
Copr build: https://copr.fedorainfracloud.org/coprs/build/6524159 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242859-rocm-rpm-macros/fedora-rawhide-x86_64/06524159-rocm-rpm-macros/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.
Looks good, approved. Feel free to just add the sources straight to the fedora src git. I agree it's better there and easier for the rocm sig to patch.
The Pagure repository was created at https://src.fedoraproject.org/rpms/rocm-rpm-macros