Bug 2259263 - Review Request: hipsolver - ROCm SOLVER marshalling library
Summary: Review Request: hipsolver - ROCm SOLVER marshalling library
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Tim Flink
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/ROCmSoftwarePlatfo...
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-01-19 20:33 UTC by Tom Rix
Modified: 2024-02-05 19:45 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2024-02-05 19:45:32 UTC
Type: ---
Embargoed:
tflink: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 6923178 to 6926729 (1.08 KB, patch)
2024-01-21 05:57 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 6926729 to 6942965 (920 bytes, patch)
2024-01-23 13:20 UTC, Fedora Review Service
no flags Details | Diff

Description Tom Rix 2024-01-19 20:33:01 UTC
Spec URL: https://trix.fedorapeople.org/hipsolver.spec
SRPM URL: https://trix.fedorapeople.org/hipsolver-6.0.0-1.fc40.src.rpm

hipSOLVER is a LAPACK marshalling library, with multiple supported                                                                                        
backends. It sits between the application and a 'worker'                                                                                                  
LAPACK library, marshalling inputs into the backend library and                                                                                           
marshalling results back to the application. hipSOLVER exports an                                                                                         
interface that does not require the client to change, regardless                                                                                          
of the chosen backend. Currently, hipSOLVER supports rocSOLVER                                                                                            
and cuSOLVER as backends.

Reproducible: Always

Comment 1 Fedora Review Service 2024-01-19 20:43:34 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.

Comment 2 Tim Flink 2024-01-20 23:43:43 UTC
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?

Comment 3 Tom Rix 2024-01-21 01:51:36 UTC
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.

Comment 4 Fedora Review Service 2024-01-21 05:57:53 UTC
Created attachment 2009529 [details]
The .spec file difference from Copr build 6923178 to 6926729

Comment 5 Fedora Review Service 2024-01-21 05:57:55 UTC
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.

Comment 6 Tim Flink 2024-01-22 18:01:27 UTC
(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.

Comment 7 Tom Stellard 2024-01-22 21:21:41 UTC
(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.

Comment 8 Tom Rix 2024-01-23 13:06:45 UTC
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.

Comment 9 Fedora Review Service 2024-01-23 13:20:57 UTC
Created attachment 2009895 [details]
The .spec file difference from Copr build 6926729 to 6942965

Comment 10 Fedora Review Service 2024-01-23 13:21:00 UTC
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.

Comment 11 Tim Flink 2024-01-23 20:14:55 UTC
(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.

Comment 12 Fedora Admin user for bugzilla script actions 2024-01-25 23:34:59 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/hipsolver


Note You need to log in before you can comment on or make changes to this bug.