https://bugzilla.redhat.com/show_bug.cgi?id=2213372
Bug ID: 2213372 Summary: Review Request: hipcc - HIP compiler driver Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: alexjnewt@fastmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://mystro256.fedorapeople.org/hipcc.spec SRPM URL: https://mystro256.fedorapeople.org/hipcc-5.5.1-1.fc39.src.rpm Description: hipcc is a compiler driver utility that will call clang or nvcc, depending on target, and pass the appropriate include and library options for the target compiler and HIP infrastructure. Fedora Account System Username: mystro256 Copr Build: https://copr.fedorainfracloud.org/coprs/mystro256/rocm-hip/build/6019100/
Background: Right now hipcc and hipcc.pl are located in the hip subpackage of rocclr. This hipcc package contains hipcc.bin, which is a c++ port of the perl script hipcc.pl. Upstream is planning to move hipcc.pl to this source and deprecate hipcc.pl, so I'm just packaging this to get ready. In the future, when the hipcc.pl is moved, I will make changes to the rocclr package accordingly.
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
Jeremy Newton alexjnewt@fastmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value CC| |trix@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |trix@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #1 from Tom Rix trix@redhat.com --- I think it is good to prepare for the deprecation of hipcc.pl. I understand the need for hipcc.bin and hipconfig.bin but the extension is confusing It would be best if hipcc and hipp.pl could go away and hipcc.bin became hipcc
calling hipcc.bin and hipconfig.bin directly both failed.
could this wait until the deprecation of hipcc.pl is completed upstream ?
Some other notes below
-Source0: https://github.com/ROCm-Developer-Tools/HIPCC/archive/refs/tags/rocm-%%7Bver... +# use %{url} to reduce generally all these instances that have it as a prefix +#Source0: https://github.com/ROCm-Developer-Tools/HIPCC/archive/refs/tags/rocm-%%7Bver... +Source0: %{url}/HIPCC/archive/refs/tags/rocm-%{version}.tar.gz#/%{name}-%{version}.tar.gz
#Upstream fixes: # HIPCC is very new, so cmake was incomplete when 5.5 was tagged: @@ -31,6 +33,8 @@ Requires: hip >= %{version} Requires: clang # 16.2 has an important fix for hipcc to work out of the box: Requires: rocm-device-libs >= 16.2 +# battling hipcc vs hipcc.bin +# Conflicts: hip <= 5.5.1
# This package depends on "hip" which is only built on these arches right now: ExclusiveArch: x86_64 aarch64 ppc64le @@ -51,12 +55,25 @@ must ensure the compiler options are appropriate for the target compiler. sed -i '/" -isystem " + hsaPath + "/include"/d' src/hipBin_amd.h
%build -%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo +# Should just use %cmake, and let rpm macros determine the build type +# %cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo +%cmake %cmake_build
%install %cmake_install
+# Needs a %check +# > hipcc.bin +# sh: line 1: /tmp/canRunUUdQtd: Is a directory +# sh: line 1: /tmp/canRuntxtm5v: Is a directory +# sh: line 1: /tmp/canRunJMKATa: Is a directory +# sh: line 1: /tmp/canRunhWTbpf: Is a directory +# Device not supported - Defaulting to AMD +# No Arguments passed, exiting ... +# +# Similar for hipconfig.bin +
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #2 from Jeremy Newton alexjnewt@fastmail.com ---
It would be best if hipcc and hipp.pl could go away and hipcc.bin became hipcc
Yeah I believe that is the plan.
could this wait until the deprecation of hipcc.pl is completed upstream ?
I think, but don't quote me on this, the plan is: 5.6 > add hipcc package with hipcc.bin as alternative to perl 5.7 > move hipcc.pl/hipcc to hipcc package, deprecate perl and default to hipcc.bin 6.0 > drop perl, delete hipcc, rename hipcc.bin to hipcc Same idea with hipconfig{,.pl,.bin} Anyway, I can hold on to this until 5.6 or 5.7 if you want, I just wanted to prepare in case there's issues.
+# use %{url} to reduce generally all these instances that have it as a prefix
Good idea, I should do this to other ROCm packages too.
+# battling hipcc vs hipcc.bin +# Conflicts: hip <= 5.5.1
This isn't quite right. "hipcc" is just a wrapper script that calls "hipcc.pl" pr "hipcc.bin". As I said above, I believe the plan is to drop hipcc and rename hipcc.bin to hipcc, same with hipconfig. I'd say it's more accurate for the hipcc package to depend on the hip package until it's moved to the hipcc source (around 5.7 ish), then I can drop the hip package and add a provides/obsoletes here.
OR I could just move everything now. Upstream has already moved everything in their development branch.
+# Needs a %check
Good point. I think hipcc is too immature and buggy in the 5.5 branch, hence the errors. When I tried to run hipcc.bin in the chroot with a simple compile test, it fails to find math.h, despite being installed, and the -isystem '/usr/include' being added.
I think we can put this review on hold and reconvene when 5.6 is released.
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #3 from Jeremy Newton alexjnewt@fastmail.com --- Ok I think we can proceed with this.
They've already separated HIPCC source from HIP sources, so I'm effectively bundling HIPCC source in the rocclr package for now.
Spec URL: https://mystro256.fedorapeople.org/hipcc.spec SRPM URL: https://mystro256.fedorapeople.org/hipcc-5.6.0-3.fc39.src.rpm Copr Build: https://copr.fedorainfracloud.org/coprs/mystro256/rocm-hip/build/6150230/
I think full deprecation will happen around ROCm 6.0, so I'll start pushing upstream to drop the .bin extension then.
Instead of a conflicts, I just handled it as a rename of the "hip" subpackage to hipcc. See rocclr.spec, I renamed "hip" to "hipcc" and copied everything over so the contents of the package should be identical other than the addition of hipcc*.bin. Then, I'll just drop that subpackage from rocclr.spec once this is accepted. I bumped the release to higher than rocclr to make sure it happens smoothly.
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/ROCm-Dev | |eloper-Tools/HIPCC
--- Comment #4 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6150245 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #5 from Tom Rix trix@redhat.com --- earlier i had asked about a %check, this is because i would expect hipcc.bin to do at least something like
touch t.hip hipcc.bin -c t.hip
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #6 from Jeremy Newton alexjnewt@fastmail.com --- Yeah the problem is that hipcc doesn't work by itself, it seems to require a backend like rocm-hip (libhipamd64 or libcuda).
Since cuda is closed source and hipcc is a build requirement for rocm-hip, I'd suggest moving the check over to rocm-hip so we can keep it simple. i.e. dependency build chain goes: clang -> hipcc -> rocm-hip
Any thoughts on that?
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #7 from Tom Rix trix@redhat.com --- hipcc.bin really doesn't run by itself, it can not handle an unknown command arg, like -v or -h. So when I was checking it out i could not find out what version of hipcc.bin I was running. When other folks try to do this they will likewise be confused.
The setup of clang arguments looks like it duplicates / should go into clang's Toolchain/ Then this would be a passthrough for amd and a wrapper for nvcc. Why is the duplicated clang logic needed ?
Maybe hipcc.bin should not be in the normal PATH if it does not behave well ? This introduces other problems so I am not sure.
It would be good if hipcc.bin were more polished / less rough.
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
--- Comment #8 from Jeremy Newton alexjnewt@fastmail.com ---
It would be good if hipcc.bin were more polished / less rough.
Should I exclude it for now, while I work with upstream to improve things?
I sort of want to get this going to clean up the already messy rocclr package.
https://bugzilla.redhat.com/show_bug.cgi?id=2213372
Jeremy Newton alexjnewt@fastmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |WONTFIX Status|ASSIGNED |CLOSED Last Closed| |2024-01-10 19:38:10
--- Comment #9 from Jeremy Newton alexjnewt@fastmail.com --- So this is obsoleted by: https://src.fedoraproject.org/rpms/rocm-compilersupport/pull-request/4
Or rather obsoleted when we move to ROCm 6.1
So I'm going to abandon this.
package-review@lists.fedoraproject.org