Bug 2335652 - Review Request: rocm-omp - ROCm OpenMP
Summary: Review Request: rocm-omp - ROCm OpenMP
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Jeremy Newton
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/ROCm/%{upstreamname}
Whiteboard:
Depends On:
Blocks: 2335650 2341286
TreeView+ depends on / blocked
 
Reported: 2025-01-05 14:21 UTC by Tom.Rix
Modified: 2025-04-10 00:22 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2025-04-10 00:22:09 UTC
Type: ---
Embargoed:
alexjnewt: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 8472432 to 8558142 (4.21 KB, patch)
2025-01-22 03:25 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 8558142 to 8578163 (476 bytes, patch)
2025-01-27 16:40 UTC, Fedora Review Service
no flags Details | Diff

Description Tom.Rix 2025-01-05 14:21:37 UTC
Spec URL: https://trix.fedorapeople.org/rocm-omp.spec
SRPM URL: https://trix.fedorapeople.org/rocm-omp-6.3.1-1.fc42.src.rpm

In F42 ROCm is using a bundled llvm in rocm-compilersupport.
The compat llvm 18 is being orphaned.

This package provides the OpenMP functionality of compat llvm.
Because it also adds offload to gpu functionality, it needs to be built after the rocm runtime.  So it needs it's own package.

This package is needed now by the rocm-rpp package.


Reproducible: Always

Comment 1 Fedora Review Service 2025-01-05 15:54:12 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8472432
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2335652-rocm-omp/fedora-rawhide-x86_64/08472432-rocm-omp/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 2 Jeremy Newton 2025-01-10 23:26:40 UTC
As a good practice, you should clean up the source you don't need in %prep if you don't need most of this.
It makes it more clear what subset you're building, and it makes the source licensing clearer.

Comment 3 Jeremy Newton 2025-01-10 23:50:29 UTC
Furthermore, you should add a rocm-llvm-filesystem package that owns the following, and have this package require it:
/usr/lib64/rocm/llvm/lib/clang
/usr/lib64/rocm/llvm/lib/cmake
/usr/lib64/rocm/llvm/lib/clang/18/include
/usr/lib64/rocm/llvm/include
/usr/lib64/rocm/llvm/lib
/usr/lib64/rocm/llvm/lib/clang/18
/usr/lib64/rocm/llvm
/usr/lib64/rocm/llvm/bin

If a user installs just rocm-omp, then uninstalls the package, it will orphan those directories. Having the filesystem package allows them to still have ownership, and dnf will auto clean up the leaf package.

I noticed it complains of:
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rocm-
     omp-static

But static actually requires devel which requires the base package. Anyway, false positive.

Also you used defines here:
     Note: %define requiring justification: %define __sourcedir openmp,
     %define _vpath_srcdir openmp
Not sure of the reason, but you need to comment it.

Finally, not sure what this rpmlint output is about:
rocm-omp-static.x86_64: E: static-library-without-symtab /usr/lib64/rocm/llvm/lib/libomptarget.devicertl.a
rocm-omp-static.x86_64: E: static-library-without-debuginfo /usr/lib64/rocm/llvm/lib/libomptarget.devicertl.a
rocm-omp-static.x86_64: E: lto-no-text-in-archive /usr/lib64/rocm/llvm/lib/libomptarget.devicertl.a
rocm-omp-devel.x86_64: E: invalid-soname /usr/lib64/rocm/llvm/lib/libomp.so libomp.so

Comment 4 Tom.Rix 2025-01-21 13:41:23 UTC
Spec URL: https://trix.fedorapeople.org/rocm-omp.spec
SRPM URL: https://trix.fedorapeople.org/rocm-omp-6.3.1-1.fc42.src.rpm

[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib64/rocm/llvm,
     /usr/lib64/rocm, /usr/lib64/rocm/llvm/lib

rocm-compilersupport / rocm-rpm-macros are being reworked so the ownership of the %libdir/rocm belongs to rocm-compilersupport, here

https://src.fedoraproject.org/rpms/rocm-compilersupport/blob/rawhide/f/rocm-compilersupport.spec#_924

%define _vpath_srcdir openmp
was simplied to 
cd openmp

rocm-omp, like the rest of the rocm packages will be also used for SUSE, and SUSE does its %cmake a little differently. 

I am not sure about errors for libomptarget.devicertl.a, it looks like this library is made up of AMDGPU objects and not normal X86 objects.

libomp.so.  This is consistent with system and compat clang, there are no versions.

libomp-19.1.6-2.fc42.x86_64 : OpenMP runtime for clang
Repo         : @System
Matched From : 
Filename     : /usr/lib64/libomp.so

libomp18-18.1.8-3.fc42.x86_64 : OpenMP runtime for clang
Repo         : rawhide
Matched From : 
Filename     : /usr/lib64/llvm18/lib/libomp.so

Comment 5 Fedora Review Service 2025-01-22 03:25:16 UTC
Created attachment 2067008 [details]
The .spec file difference from Copr build 8472432 to 8558142

Comment 6 Fedora Review Service 2025-01-22 03:25:18 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8558142
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2335652-rocm-omp/fedora-rawhide-x86_64/08558142-rocm-omp/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 7 Jeremy Newton 2025-01-27 02:53:37 UTC
Just realised some files are apache licensed.

We might need it in rocm-llvm too:
https://download.copr.fedorainfracloud.org/results/%40fedora-review/fedora-review-2335652-rocm-omp/fedora-rawhide-x86_64/08558142-rocm-omp/fedora-review/licensecheck.txt

Can you give it a quick skim?

Other than that, it seems fine. Some false positives in the fedora review output, but it seems fine.

Comment 8 Tom.Rix 2025-01-27 16:02:39 UTC
Spec URL: https://trix.fedorapeople.org/rocm-omp.spec
SPRM URL: https://trix.fedorapeople.org/rocm-omp-6.3.1-1.fc42.src.rpm

Change is to copy the license from the old libomp package
https://src.fedoraproject.org/rpms/libomp/blob/f40/f/libomp.spec#_41
Apache-2.0 WITH LLVM-exception OR NCSA

Then for ROCm parts add
AND MIT

Comment 9 Fedora Review Service 2025-01-27 16:40:02 UTC
Created attachment 2074043 [details]
The .spec file difference from Copr build 8558142 to 8578163

Comment 10 Fedora Review Service 2025-01-27 16:40:04 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8578163
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2335652-rocm-omp/fedora-rawhide-x86_64/08578163-rocm-omp/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 Fedora Admin user for bugzilla script actions 2025-01-27 19:49:28 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rocm-omp


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