https://bugzilla.redhat.com/show_bug.cgi?id=1545479 --- Comment #10 from Tom Stellard <tstellar@xxxxxxxxxx> --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00857850-hcc/hcc.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-rawhide-x86_64/00857850-hcc/hcc-1.3.18505-2.rocm2.0.0.fc30.src.rpm (In reply to Felix Schwarz from comment #9) > Hi Tom, fixing all the small issues I brought up (I had hoped someone more > experienced than me would have reviewed the package by now). > > I'm still hesitant to formally reviewing this package but I think I found > some more issues: > > hcc package > =============== > > - contains a folder "/usr/include/experimental" which should probably be > stored as "/usr/include/hcc/experimental" I've fixed this. > e.g. "algorithm" (no extension?) refers to "../hcc.hpp" > - empty "/lib" folder? There are hidden build-id files in /lib that were generated by RPM. > > - hcc-config.cmake contains references to "/opt/rocm" (should be > /usr/include/hcc?) > - also there it contains something like > set_and_check( hcc_INCLUDE_DIR "${PACKAGE_PREFIX_DIR}/include" ) > - I'm not sure where this cmake file is actually used but I suspect this > should point to "${PACKAGE_PREFIX_DIR}/include/hcc" as well I've fixed this. > - are the "cmake" files actually required by hcc directly? These are meant to be used by other programs, but I'm not sure what actually uses them. > > - hcc.spec: > for f in clamp-device clamp-embed clamp-assemble clamp-link > hc-kernel-assemble hc-host-assemble error-check; do > mv %{buildroot}{/usr,%{_libexecdir}/hcc}/bin/$f > done > I'd prefer "install --preserve-timestamps" > I updated the spec so these are installed by 'make install' now. > > hcc-runtime package > ===================== > - empty "/lib" folder? Has build-id files in it. > - does not install a license file! I've fixed this. > > > rocm-device-libs > ===================== > - does not install a license file! > Fixed this. > > hcc-runtime-devel > ===================== > > Note: Package has .a files: hcc-runtime-devel. Does not provide -static: > hcc-runtime-devel. > -> I assume you are aware of the static library and it is actually required > by some part of the ROCm stack > -> Fedora guidelines say that the static library MUST be in a -static > package > (https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static- > libraries) > I've replaced the static library with a shared library. > > ---------- > I was unable to check the license of all sources (due to the huge number of > sources files + the perl check script taking forever) so far. Also several > other points which I could not check yet. > > I'm also not sure if llvm counts as a bundled library and must be declared > as such > (https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling), see > also Fedora's "rust" package. I've added Provides: bundled(llvm) > Tom: I assume the rocm stack currently does not work against any upstream > version of llvm, right? Is there an official way of trying to build with a > vanilla llvm? I think it would likely work with upstream LLVM, however the ROCm release don't line up with upstream LLVM releases. So what is packaged now in hcc is a snapshot from LLVM SVN, and we only ship official llvm release in Fedora. > > My plan is to work through the rest of the review items (though that will > take some time) and also check your "hip" package in parallel. That way I > hope to validate the packaging decisions made in the "hcc" package. Ok, thanks for the review. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
