Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00848050-hip/hip.spec 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
just fyi: SRPM is likely at https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00848050-hip/hip-1.5.18494-1.rocm2.0.0.fc30.src.rpm
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00848050-hip/hip.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00848050-hip/hip-1.5.18494-1.rocm2.0.0.fc30.src.rpm Fixed SRPM link.
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00849802-hip/hip.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00849802-hip/hip-1.5.18494-1.rocm2.0.0.fc30.src.rpm Fixed include paths so hcc headers can be found now that they are only installed into /usr/include/hcc.
unfortunately both links in comment 3 are dead (404). Can you please upload them again/trigger a new build?
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00862313-hip/hip.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00862313-hip/hip-1.5.18494-1.rocm2.0.0.fc30.src.rpm Updated links.
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 :-)
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00873694-hip/hip.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00873694-hip/hip-1.5.18494-1.rocm2.0.0.fc31.src.rpm (In reply to Felix Schwarz from comment #6) > 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'; > Fixed these 2. > 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?). 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. > 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'] > Fixed. > 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) > Fixed. > 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"?) > Fixed. > 7. 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(). > 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 > Fixed. > > 9. 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 :-)
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/thread/BDPFWPWE7SKOHCEYF6T6QP27UU73SPPV/ 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.)
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.
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/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". Requires: bintuils Typo: "binutils" I tried to compile "vectoradd_hip.cpp" (https://github.com/ROCm-Developer-Tools/HIP-Examples/blob/master/vectorAdd/vectoradd_hip.cpp) 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-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.
-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
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.
> 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.
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.
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00944399-hip/hip.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00944399-hip/hip-1.5.18494-1.rocm2.0.0.fc31.src.rpm 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.
> 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.
Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00944691-hip/hip.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00944691-hip/hip-1.5.18494-1.rocm2.0.0.fc31.src.rpm I've added the missing dependency.
LGTM, package approved.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/hip
Please close your Review Requests when they are complete.