Bug 2259263
| Summary: | Review Request: hipsolver - ROCm SOLVER marshalling library | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Tom Rix <trix> | ||||||
| Component: | Package Review | Assignee: | Tim Flink <tflink> | ||||||
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||
| Severity: | medium | Docs Contact: | |||||||
| Priority: | unspecified | ||||||||
| Version: | rawhide | CC: | package-review, rocm-packagers-sig, tflink, tstellar | ||||||
| Target Milestone: | --- | Flags: | tflink:
fedora-review+
|
||||||
| Target Release: | --- | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Linux | ||||||||
| URL: | https://github.com/ROCmSoftwarePlatform/%{upstreamname} | ||||||||
| Whiteboard: | |||||||||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |||||||
| Doc Text: | Story Points: | --- | |||||||
| Clone Of: | Environment: | ||||||||
| Last Closed: | 2024-02-05 19:45:32 UTC | Type: | --- | ||||||
| Regression: | --- | Mount Type: | --- | ||||||
| Documentation: | --- | CRM: | |||||||
| Verified Versions: | Category: | --- | |||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||
| Embargoed: | |||||||||
| Attachments: |
|
||||||||
|
Description
Tom Rix
2024-01-19 20:33:01 UTC
Copr build: https://copr.fedorainfracloud.org/coprs/build/6923178 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2259263-hipsolver/fedora-rawhide-x86_64/06923178-hipsolver/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 few small things: 1. "BuildRequires: clang" to satisfy https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ 2. all the rocm subprojects were consolidated into a single github project and the base url needs to be updated- https://github.com/ROCm/ 3. Source0 should drop the /refs/tags/ per https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags 4. shouldn't CMAKE_BUILD_TYPE be RelWithDebInfo ? The larger issue(s) is that I can't get the tests to pass on my machine. Most, if not all of them fail but I get a segfault before the suite finishes. Are you able to get the tests to pass? Spec URL: https://trix.fedorapeople.org/hipsolver.spec SRPM URL: https://trix.fedorapeople.org/hipsolver-6.0.0-2.fc40.src.rpm 1. clang-devel pulls in clang. 2,3,4 I have made these changes in the specfile Testing. rocminfo reports i am using mi210, a pretty new datacenter card. Name: gfx90a install hipsolver-test From the source dir hipSOLVER/clients/sparsedata run > module load rocm/gfx9 > /usr/lib64/rocm/gfx9/bin/hipsolver-test and i get ... [==========] 9420 tests from 82 test suites ran. (16312 ms total) [ PASSED ] 9420 tests. hipSOLVER version 2.0.0. They all pass. Created attachment 2009529 [details]
The .spec file difference from Copr build 6923178 to 6926729
Copr build: https://copr.fedorainfracloud.org/coprs/build/6926729 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2259263-hipsolver/fedora-rawhide-x86_64/06926729-hipsolver/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. (In reply to Tom Rix from comment #3) > Spec URL: https://trix.fedorapeople.org/hipsolver.spec > SRPM URL: https://trix.fedorapeople.org/hipsolver-6.0.0-2.fc40.src.rpm > > 1. clang-devel pulls in clang. I agree that it's redundant and probably not what the guidelines meant to enforce but that doesn't change what the packaging guidelines say. "If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang." > run > > module load rocm/gfx9 > > /usr/lib64/rocm/gfx9/bin/hipsolver-test > > and i get > ... > [==========] 9420 tests from 82 test suites ran. (16312 ms total) > [ PASSED ] 9420 tests. > hipSOLVER version 2.0.0. > > They all pass. If I do a local build (not with mock) while QA_RPATHS=0xff, I can get the tests to pass using that so long as CWD is /usr/share/hipsolver/test. it's not ideal and I'm not crazy about the process but it's not a package review issue. (In reply to Tim Flink from comment #6) > (In reply to Tom Rix from comment #3) > > Spec URL: https://trix.fedorapeople.org/hipsolver.spec > > SRPM URL: https://trix.fedorapeople.org/hipsolver-6.0.0-2.fc40.src.rpm > > > > 1. clang-devel pulls in clang. > > I agree that it's redundant and probably not what the guidelines meant to > enforce but that doesn't change what the packaging guidelines say. > > "If your application is a C or C++ application you must list a BuildRequires > against gcc, gcc-c++ or clang." > The other reason to explicitly BuildRquires: clang is because clang-devel may not always pull in clang. Right now clang-devel pulling in clang is just a workaround for an issue with the CMake export files, but it's something we would like to fix. Spec URL: https://trix.fedorapeople.org/hipsolver.spec SRPM URL: https://trix.fedorapeople.org/hipsolver-6.0.0-1.fc40.src.rpm For the clang change. I also switched to autorelease, autochangelog as that is how other rocm's have been moved to. Created attachment 2009895 [details]
The .spec file difference from Copr build 6926729 to 6942965
Copr build: https://copr.fedorainfracloud.org/coprs/build/6942965 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2259263-hipsolver/fedora-rawhide-x86_64/06942965-hipsolver/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 Tom Stellard from comment #7) > (In reply to Tim Flink from comment #6) > > (In reply to Tom Rix from comment #3) > > > Spec URL: https://trix.fedorapeople.org/hipsolver.spec > > > SRPM URL: https://trix.fedorapeople.org/hipsolver-6.0.0-2.fc40.src.rpm > > > > > > 1. clang-devel pulls in clang. > > > > I agree that it's redundant and probably not what the guidelines meant to > > enforce but that doesn't change what the packaging guidelines say. > > > > "If your application is a C or C++ application you must list a BuildRequires > > against gcc, gcc-c++ or clang." > > > > The other reason to explicitly BuildRquires: clang is because clang-devel > may not always pull in clang. Right now clang-devel pulling in clang is > just a workaround for an issue with the CMake export files, but it's > something we would like to fix. Good to know, thanks. Package LGTM. I'd suggest putting -b 3 after autorelease to make sure that the official builds supercede any of the previous review versions but it's not critical. The Pagure repository was created at https://src.fedoraproject.org/rpms/hipsolver |