https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Bug ID: 1668010 Summary: Review Request: hip- Tool for porting CUDA to Portable C++ Code Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: tstellar@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: hip-1.5.18494-1.rocm2.0.0.fc30.src.rpm Description: hip is part of the Radeon Open Compute (ROCm) ecosystem and is a tool for porting CUDA to Portable C++ Code. Fedora Account System Username: tstellar
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Tom Stellard tstellar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fschwarz@fedoraproject.org Depends On| |1545479
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1545479 [Bug 1545479] Review Request: hcc- Heterogeneous C++ Compiler
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #1 from Felix Schwarz fschwarz@fedoraproject.org --- just fyi: SRPM is likely at https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #2 from Tom Stellard tstellar@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
Fixed SRPM link.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #3 from Tom Stellard tstellar@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
Fixed include paths so hcc headers can be found now that they are only installed into /usr/include/hcc.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #4 from Felix Schwarz fschwarz@fedoraproject.org --- unfortunately both links in comment 3 are dead (404). Can you please upload them again/trigger a new build?
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #5 from Tom Stellard tstellar@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
Updated links.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #6 from Felix Schwarz fschwarz@fedoraproject.org --- 1. "LICENSE" file not installed (%license)
2. /usr/bin/hipconfig contains some unpatched paths: $CUDA_PATH=$ENV{'CUDA_PATH'} // '/usr/local/cuda'; $HCC_HOME=$ENV{'HCC_HOME'} // '/opt/rocm/hcc'; $HSA_PATH=$ENV{'HSA_PATH'} // '/opt/rocm/hsa';
3. /usr/bin/hipcc - unpatched paths - HIP_PATH defaults to "/usr" but $HIP_INCLUDE_PATH is defined as "$HIP_PATH/include" (-> $HIP_PATH/include/hip?). - from a quick glance it seems as if »$HIP_PLATFORM = "clang"« won't work as no default path is set? (just from reading the code) - missing requires: hipcc requires coreutils, grep, gcc, gcc-c++ (the latter two are only required for a CentOS 7-specific workaround)
4. /usr/lib64/libhip_hcc.so.1.5 - executable-stack - E: binary-or-shlib-defines-rpath /usr/lib64/libhip_hcc.so.1.5 ['/usr/lib64']
5. hipexamine.sh, hipconvertinplace.sh non-functional - "hipify-clang" is no built (not built by default), both scripts rely on that - probably best to remove these scripts entirely (I guess there is a reason why upstream does not build them by default)
6. In general I find that the hip package "clutters" /usr/bin/ a bit too much. Things like 'findcode.sh' could go into "libexec" (but that might be just personal preference). Also I'm a bit worried about the "ca" binary - that name is extremly generic ("certificate authority"?)
7. from a quick glance it seems as if "clara.hpp" is a header-only copylib. "bundles(clara)"?
8. license tag only lists MIT but licensecheck disagrees: *No copyright* GNU Lesser General Public License (v2.1 or later) ---------------------------------------------------------------- HIP-roc-2.0.0/util/gedit/hip.lang
BSD 3-clause "New" or "Revised" License --------------------------------------- HIP-roc-2.0.0/samples/1_Utils/hipBusBandwidth/LICENSE.txt HIP-roc-2.0.0/samples/1_Utils/hipCommander/LICENSE.txt
BSL (v1.0) ---------- HIP-roc-2.0.0/lpl_ca/pstreams/pstream.h
9. hipdemangleatp.sh: missing requires: - sed, binutils, coreutils
I have to say that I find "hip" even scarier to review with its static libraries and all the duplicated shell/perl code...
Also I did not have time yet to actually try the hip so there may be more issues. Anyway thanks for your work so far :-)
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Tom Stellard tstellar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: hip- Tool |Review Request: hip - Tool |for porting CUDA to |for porting CUDA to |Portable C++ Code |Portable C++ Code
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #7 from Tom Stellard tstellar@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
(In reply to Felix Schwarz from comment #6)
"LICENSE" file not installed (%license)
/usr/bin/hipconfig contains some unpatched paths:
$CUDA_PATH=$ENV{'CUDA_PATH'} // '/usr/local/cuda'; $HCC_HOME=$ENV{'HCC_HOME'} // '/opt/rocm/hcc'; $HSA_PATH=$ENV{'HSA_PATH'} // '/opt/rocm/hsa';
Fixed these 2.
- /usr/bin/hipcc - unpatched paths
- HIP_PATH defaults to "/usr" but $HIP_INCLUDE_PATH is defined as
"$HIP_PATH/include" (-> $HIP_PATH/include/hip?).
I think $HIP_PATH/include is correct, because the test cases I've seen use the hip/ prefix in #include directives.
- from a quick glance it seems as if »$HIP_PLATFORM = "clang"« won't work
as no default path is set? (just from reading the code)
Right, you would need to set the HIP_CLANG_PATH environment variable to make it work. This is by design, since I couldn't get it to work with clang-7.0.0. Long-term, I hope to make HIP_PLATFORM=clang the default once I can get it working.
- missing requires: hipcc requires coreutils, grep, gcc, gcc-c++ (the
latter two are only required for a CentOS 7-specific workaround)
Fixed.
- /usr/lib64/libhip_hcc.so.1.5
- executable-stack
- E: binary-or-shlib-defines-rpath /usr/lib64/libhip_hcc.so.1.5
['/usr/lib64']
Fixed.
- hipexamine.sh, hipconvertinplace.sh non-functional
- "hipify-clang" is no built (not built by default), both scripts rely on
that
- probably best to remove these scripts entirely (I guess there is a reason
why upstream does not build them by default)
Fixed.
- In general I find that the hip package "clutters" /usr/bin/ a bit too
much. Things like 'findcode.sh' could go into "libexec" (but that might be just personal preference). Also I'm a bit worried about the "ca" binary - that name is extremly generic ("certificate authority"?)
Fixed.
- from a quick glance it seems as if "clara.hpp" is a header-only copylib.
"bundles(clara)"?
I wasn't sure what to do here. There is no clara package in Fedora, so I'm not sure there is any reason to do bundle().
license tag only lists MIT but licensecheck disagrees:
*No copyright* GNU Lesser General Public License (v2.1 or later)
HIP-roc-2.0.0/util/gedit/hip.lang
BSD 3-clause "New" or "Revised" License
HIP-roc-2.0.0/samples/1_Utils/hipBusBandwidth/LICENSE.txt HIP-roc-2.0.0/samples/1_Utils/hipCommander/LICENSE.txt
BSL (v1.0)
HIP-roc-2.0.0/lpl_ca/pstreams/pstream.h
Fixed.
- hipdemangleatp.sh: missing requires:
- sed, binutils, coreutils
Fixed.
I have to say that I find "hip" even scarier to review with its static libraries and all the duplicated shell/perl code...
Also I did not have time yet to actually try the hip so there may be more issues. Anyway thanks for your work so far :-)
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #8 from Felix Schwarz fschwarz@fedoraproject.org --- Hi Tom,
seems like nobody is willing to review this... Anyway we had a ROCm 2.4 release recently while this package is still based on ROCm 2.0. Do you think it will be useful to upgrade this package before a full review or is that just unnecessary churn?
Regarding the "clara" bundling I think this is needed as per Fedora's packaging policy ( https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling ). See also: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/...
I still need some time to clean up some other Fedora package (borgbackup, bug 1669083) but I plan to go through this package afterwards (and will probably nudge a few people to either review my notes or do the formal review). I'm really looking forward to having the ROCm stack available on Fedora easily. (Usual disclaimer: If someone else is ready to review this please just do.)
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #9 from Tom Stellard tstellar@redhat.com --- Ok, I'll fix the bundling issue. I think it's easiest to keep this at ROCm 2.0 until it is reviewed and then upgrade the whole stack together.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #10 from Felix Schwarz fschwarz@fedoraproject.org --- License: MIT and GPL and BSD and BSL
I think "GPL" was added because of "hip.lang" for gedit. "BSD" is used for two example applications in "samples". That code is not shipped in the final binaries as far as I can see so it should not be present in the License tag.
"BSL" probably refers to the boost software license (short name "Boost").
-> License: MIT and Boost
Also https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline... says:
In addition, the package must contain a comment explaining the multiple licensing breakdown.
Only pstream.h uses the boost license and only "lpl" includes it so it should be easy to document in "%files" that lpl is under "MIT AND Boot".
Requires: bintuils
Typo: "binutils"
I tried to compile "vectoradd_hip.cpp" (https://github.com/ROCm-Developer-Tools/HIP-Examples/blob/master/vectorAdd/v...) with hipcc which caused this error:
$ hipcc `hipconfig --cpp_config` vectoradd_hip.cpp -o vectoradd_hip In file included from vectoradd_hip.cpp:27: In file included from /usr//include/hip/hip_runtime.h:56: In file included from /usr//include/hip/hcc_detail/hip_runtime.h:96: In file included from /usr//include/hip/hcc_detail/grid_launch_GGL.hpp:26: In file included from /usr//include/hip/hcc_detail/functional_grid_launch.hpp:25: /usr//include/hip/hcc_detail/code_object_bundle.hpp:25:10: fatal error: 'hsa/hsa.h' file not found #include <hsa/hsa.h> ^~~~~~~~~~~ 1 error generated.
Unless I made a mistake (using F30 with a custom recompile of the rawhide SRPMs) I think you also must depend on "rocm-runtime-devel".
My biggest gripe is how to deal with "libhip_hcc_static.a" properly (https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-...). A -devel package does not make much sense for hip and as far as I can see upstream does not provide a way to prevent static linking with hipcc.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com
--- Comment #11 from Robert-André Mauchin zebob.m@gmail.com --- -DHCC_HOME=/usr/ \ -DHSA_PATH=/usr/ \
Use %{_prefix} here
- The static lib should probably not be shipped. Or if you need it, ship it in a static subpackage.
- You need to put the include, cmake files, and unversioned library into a devel subpackage. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #12 from Tom Stellard tstellar@redhat.com --- Spec URL: https://fedorapeople.org/~tstellar/hip.spec SRPM URL: https://fedorapeople.org/~tstellar/hip-1.5.18494-1.rocm2.0.0.fc31.src.rpm
Added bundled(clara)
(In reply to Felix Schwarz from comment #10)
-> License: MIT and Boost
Fixed.
Also https://docs.fedoraproject.org/en-US/packaging-guidelines/ LicensingGuidelines/#_multiple_licensing_scenarios says:
In addition, the package must contain a comment explaining the multiple licensing breakdown.
Only pstream.h uses the boost license and only "lpl" includes it so it should be easy to document in "%files" that lpl is under "MIT AND Boot".
Added a comment for this.
Requires: bintuils
Typo: "binutils"
Fixed this.
Unless I made a mistake (using F30 with a custom recompile of the rawhide SRPMs) I think you also must depend on "rocm-runtime-devel".
I split hip into hip-runtime and hip-runtime-devel packages like I did with hcc, so hip no requires hip-runtime-devel which requires rocm-runtimed-devel.
My biggest gripe is how to deal with "libhip_hcc_static.a" properly (https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static- libraries). A -devel package does not make much sense for hip and as far as I can see upstream does not provide a way to prevent static linking with hipcc.
I've deleted the static library.
(In reply to Robert-André Mauchin from comment #11)
-DHCC_HOME=/usr/ \ -DHSA_PATH=/usr/ \
Use %{_prefix} here
This has been fixed.
- You need to put the include, cmake files, and unversioned library into a
devel subpackage. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
I've added a hip-runtime-devel sub-package and moved the mentioned files there.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #13 from Felix Schwarz fschwarz@fedoraproject.org ---
I've deleted the static library.
$ hipcc -use-staticlib `hipconfig --cpp_config` vectoradd_hip.cpp -o vectoradd_hip hcc: error: no such file or directory: '/usr/lib64/libhip_hcc_static.a'
So the error message is better than expected (and probably nobody uses hip+static linking) but I just wanted to mention that hipcc assumes the static lib is always present.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #14 from Felix Schwarz fschwarz@fedoraproject.org --- One more thing: If you delete the static library you also need to fix /usr/lib64/cmake/hip/hip-targets.cmake which contains references to "libhip_hcc_static.a". I noticed that while trying to build rocblas with your hip package.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #15 from Tom Stellard tstellar@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
I decided to add back the static library in its own sub-package. I don't want to remove the -use-staticlib option, because it seems like it might cause us problems in the future if fedora is missing on option supported by upstream.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #16 from Felix Schwarz fschwarz@fedoraproject.org ---
I decided to add back the static library in its own sub-package. I don't want to remove the -use-staticlib option, because it seems like it might cause us problems in the future if fedora is missing on option supported by upstream.
Agreed.
However I think you forgot to add a dependency. Output when building rocblas:
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.6.1") CMake Error at /usr/lib64/cmake/hip/hip-targets.cmake:95 (message): The imported target "hip::hip_hcc_static" references the file
"/usr/lib64/libhip_hcc_static.a"
but this file does not exist. Possible reasons include:
So I think hip-runtime-devel (which owns hip-targets.cmake) should also depend on hip-runtime-static.
@Robert-André: Do you think the current solution with the static subpackage + hard dependency from hip-runtime-devel is ok? As far as I can tell this package seems to be fine so I'm willing to approve that package.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #17 from Tom Stellard tstellar@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
I've added the missing dependency.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |POST Assignee|nobody@fedoraproject.org |zebob.m@gmail.com Flags| |fedora-review+
--- Comment #18 from Robert-André Mauchin zebob.m@gmail.com --- LGTM, package approved.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
--- Comment #19 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/hip
https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED CC| |quantum.analyst@gmail.com Fixed In Version| |hip-1.5.18494-1.rocm2.0.0.f | |c31 Resolution|--- |CURRENTRELEASE Last Closed| |2020-08-03 00:34:49
--- Comment #20 from Elliott Sales de Andrade quantum.analyst@gmail.com --- Please close your Review Requests when they are complete.
https://bugzilla.redhat.com/show_bug.cgi?id=1668010 Bug 1668010 depends on bug 1545479, which changed state.
Bug 1545479 Summary: Review Request: hcc - Heterogeneous C++ Compiler https://bugzilla.redhat.com/show_bug.cgi?id=1545479
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |CURRENTRELEASE
package-review@lists.fedoraproject.org