Spec URL: https://trix.fedorapeople.org/miopen.spec SRPM URL: https://trix.fedorapeople.org/miopen-6.0.0-1.fc40.src.rpm AMD's library for high performance machine learning primitives. Needed for PyTorch Reproducible: Always
Copr build: https://copr.fedorainfracloud.org/coprs/build/6971829 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2261201-miopen/fedora-rawhide-x86_64/06971829-miopen/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Yay, public domain fun times. As near as I can tell, the text of the license blerb in src/md5.cpp needs to be filed with other public domain blerbs. https://docs.fedoraproject.org/en-US/legal/update-existing-packages/#_public_domain That being said, I'm a little unclear on if the file is still public domain since it has been modified from the original but the license header for the file isn't changed so it probably still is? The big thing I'm seeing is the pre-compiled kernels that are in the -devel package (/usr/share/miopen/db). Was that intentional? If so, doesn't that violate packaging guidelines? A few small-ish things: - shouldn't URL be https://github.com/ROCm/%{upstreamname} ? - shouldn't CMAKE_BUILD_TYPE be RelWithDebInfo? - BuildRequires should be pkgconfig instead of explicit XXX-devel - please add notes for what those patches are for and if they're intended to be long-term vs. temporary * especially in 0001-prepare-miopen-for-fedora.patch; I think I know what those bits are for but I'm not sure What is the purpose of compiler/linker patch and math to specify the number of compile and link jobs? I tried removing it and it builds without issue, is it a speedup?
(In reply to Tim Flink from comment #2) > Yay, public domain fun times. As near as I can tell, the text of the license > blerb in src/md5.cpp needs to be filed with other public domain blerbs. > > https://docs.fedoraproject.org/en-US/legal/update-existing-packages/ > #_public_domain > > That being said, I'm a little unclear on if the file is still public domain > since it has been modified from the original but the license header for the > file isn't changed so it probably still is? > > The big thing I'm seeing is the pre-compiled kernels that are in the -devel > package (/usr/share/miopen/db). Was that intentional? If so, doesn't that > violate packaging guidelines? can you explain what you mean here ? > > A few small-ish things: > - shouldn't URL be https://github.com/ROCm/%{upstreamname} ? > - shouldn't CMAKE_BUILD_TYPE be RelWithDebInfo? > - BuildRequires should be pkgconfig instead of explicit XXX-devel can you explain what you mean here ? this follows what I been doing for all the other rocm packages. > - please add notes for what those patches are for and if they're intended > to be long-term vs. temporary > * especially in 0001-prepare-miopen-for-fedora.patch; I think I know what > those bits are for but I'm not sure > > What is the purpose of compiler/linker patch and math to specify the number > of compile and link jobs? I tried removing it and it builds without issue, > is it a speedup? it is to prevent thrashing the building when the number of compile and/or link jobs use too much memory. once we tip over to virtual memory the machine is effectively crippled and if the build completes it will be days or weeks later. i have done this for a number of other rocm packages with similar problems.
(In reply to Tom Rix from comment #3) <snip> > > The big thing I'm seeing is the pre-compiled kernels that are in the -devel > > package (/usr/share/miopen/db). Was that intentional? If so, doesn't that > > violate packaging guidelines? > > can you explain what you mean here ? Aren't the kernels in /usr/share/miopen/db just bunziped from upstream (src/kernels/) or am I reading the build logs incorrectly? > > A few small-ish things: > > - shouldn't URL be https://github.com/ROCm/%{upstreamname} ? > > - shouldn't CMAKE_BUILD_TYPE be RelWithDebInfo? > > - BuildRequires should be pkgconfig instead of explicit XXX-devel > > can you explain what you mean here ? this follows what I been doing for all > the other rocm packages. I think that I've missed the pkgconfig part for reviews that I've done; sorry about that. I even left BuildRequires on *-devel for all the BuildRequires in rocfft which is my bad and I'm going to fix. https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/ From a quick scan through the spec file, it's just some of the BuildRequires, though. bzip2-devel -> pkgconfig(bzip2) eigen3-devel -> pkgconfig(eigen3) libstd-devel -> pkgconfig(libzstd) json-devel -> pkgconfig(nlohmann_json) sqlite-devel -> pkgconfig(sqlite3) zlib-devel -> zlib-ng-devel -> pkgconfig(zlib-ng) # zlib was retired from rawhide and replaced by zlib-ng <snip> > > What is the purpose of compiler/linker patch and math to specify the number > > of compile and link jobs? I tried removing it and it builds without issue, > > is it a speedup? > > it is to prevent thrashing the building when the number of compile and/or > link jobs use too much memory. once we tip over to virtual memory the > machine is effectively crippled and if the build completes it will be days > or weeks later. i have done this for a number of other rocm packages with > similar problems. Ah, OK. My local machine has 64GB so I must have been able to avoid the problem.
Spec URL: https://trix.fedorapeople.org/miopen.spec SRPM URL: https://trix.fedorapeople.org/miopen-6.0.0-1.fc40.src.rpm Most of the issues are addressed. The db's. I am not sure what you saying. can we not use a db as-is ? For patch documenting, I have added comments to several issues i have opened in the upstream project. Generally this project assumes it is built from the amd rpms and has a lot of rough edges the older rocm projects have smoothed off. So I feel the main problem is the upstream needs to try to build on a second install location and they will understand and fix their problems. This issue is an example of that https://github.com/ROCm/MIOpen/issues/2734
Created attachment 2016149 [details] The .spec file difference from Copr build 6971829 to 7005485
Copr build: https://copr.fedorainfracloud.org/coprs/build/7005485 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2261201-miopen/fedora-rawhide-x86_64/07005485-miopen/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Spec URL: https://trix.fedorapeople.org/miopen.spec SRPM URL: https://trix.fedorapeople.org/miopen-6.0.2-1.fc40.src.rpm Updated to 6.0.2 so I could try out a fix proposed by AMD, fix looks good. Changed and documented some hackery for the preparing the cmake infra. Tested on the in-dev pytorch+rocm
Created attachment 2016657 [details] The .spec file difference from Copr build 7005485 to 7012168
Copr build: https://copr.fedorainfracloud.org/coprs/build/7012168 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2261201-miopen/fedora-rawhide-x86_64/07012168-miopen/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Spec URL: https://trix.fedorapeople.org/miopen.spec SRPM URL: https://trix.fedorapeople.org/miopen-6.0.2-1.fc40.src.rpm The prebuilt db's have been removed.
Created attachment 2016779 [details] The .spec file difference from Copr build 7012168 to 7018175
Copr build: https://copr.fedorainfracloud.org/coprs/build/7018175 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2261201-miopen/fedora-rawhide-x86_64/07018175-miopen/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.
I realize that the test binaries aren't being built normally but why make the test binaries arch specific instead of building it for all arches like the library is built? However, I'm not sure how to handle the license file text since upstream only includes MIT text. I've asked about how this has been handled in the past: https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/message/FJCX5VM5XQP3Y3K4SY25SDAAX6ULTQEA/ (In reply to Tom Rix from comment #5) <snip> > The db's. I am not sure what you saying. can we not use a db as-is ? This may be pointless since you've already removed the files, but aren't the db files binaries distributed by upstream? That would violate bundling policies unless I'm missing something here. > For patch documenting, I have added comments to several issues i have opened > in the upstream project. Generally this project assumes it is built from > the amd rpms and has a lot of rough edges the older rocm projects have > smoothed off. So I feel the main problem is the upstream needs to try to > build on a second install location and they will understand and fix their > problems. Thanks, I appreciate it. Overall, it looks good to me. The only thing left in my mind is the details around how we include licensing text.
I would have liked to include a -test package, it had problems so I fell back on testing during the build. This is one of the rough edges. I thought the guidance on complicated licensing was to comment in the spec file. All of the licenses themselves are acceptable.
(In reply to Tom Rix from comment #15) > I would have liked to include a -test package, it had problems so I fell > back on testing during the build. This is one of the rough edges. OK, we can figure out better test packages later if that becomes possible. It's not really blocking and more of an issue for (automated) testing > I thought the guidance on complicated licensing was to comment in the spec > file. > All of the licenses themselves are acceptable. Yeah, I wasn't concerned about the licenses. I just wasn't sure whether the tags were sufficient for the BSD and Apache licenses which both stipulate that their copyright notice be included with any source or binary distributions. If the license text is only in source files, that text isn't included with distributed binaries. After a thread on the packaging list [1], it sounds like this subject is kinda known to be confusing at times and there isn't really a clear answer on what is supposed to happen. From what I gather, the most common approach in cases like this where the license file is upstream but doesn't include all the licenses actually used is to include upstream's license text file, file an issue requesting that upstream update their license file. [1] https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/FJCX5VM5XQP3Y3K4SY25SDAAX6ULTQEA/#FJCX5VM5XQP3Y3K4SY25SDAAX6ULTQEA Since I'm the one being a pain about this, I went ahead and filed an issue [2] and PR [3] with the MIOpen upstream addressing the incomplete LICENSE.txt file. [2] https://github.com/ROCm/MIOpen/issues/2757 [3] https://github.com/ROCm/MIOpen/pull/2758 This issue isn't enough to block review, though.
The Pagure repository was created at https://src.fedoraproject.org/rpms/miopen