Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few packaging improvements #1

Merged
merged 3 commits into from
Sep 7, 2022
Merged

Conversation

Mystro256
Copy link
Contributor

As I mentioned in another thread, hipinfo shouldn't be necessary, and installing data files into libdir is counter to Fedora's FHS policy. You can discard the commit if you wish, but for strictness, if we're going to try to get this into Fedora proper, I would suggest discarding it for now.

I noticed rpmlint complains about a few other files, but they cannot be discarded as far as I know:
rocm-hip-devel.x86_64: E: script-without-shebang /usr/bin/.hipVersion
rocm-hip-devel.x86_64: E: script-without-shebang /usr/bin/hipvars.pm
rocm-hip-devel.x86_64: W: non-executable-in-bin /usr/bin/.hipVersion 644
rocm-hip-devel.x86_64: W: non-executable-in-bin /usr/bin/hipvars.pm 664

@cgmb was working with Debian to resolve these, but he might not be available to provide input right now. He might be a better contact for hipcc related issues. I believe he works on a lot of the HIP libraries needed for applications such as pytorch.

Also, the samples package should be noarch since it's just source code. I noticed rpmlint still warns about these files, e.g.:

rocm-hip-samples.noarch: W: devel-file-in-non-devel-package /usr/share/hip/samples/0_Intro/bit_extract/bit_extract.cpp
rocm-hip-samples.noarch: W: devel-file-in-non-devel-package /usr/share/hip/samples/0_Intro/module_api/defaultDriver.cpp
... etc.

But I feel like this is a false positive and can be ignored.

I was speaking to upstream and it seems like they want to merge all of the source into a single repo (hipamd, rocclr, opencl), so maybe in the longterm we can merge the hip package into Fedora's rocm-opencl, or add a new parent package and make opencl and hip subpackages.

This is not strictly needed and fixes a rpmlint warning.
This package contains no binaries.
I don't believe this source is arch specific.
Github seems to mangle these permissions, so since the files are copied
as-is, we should just make sure they're set right from the beginning.
@Mystro256
Copy link
Contributor Author

One more patch: it looks like there's some permission issues.

I think github is mangling these, because I'm not getting these permissions after a git clone.

@awehrfritz
Copy link
Owner

awehrfritz commented Sep 7, 2022

Thanks for this @Mystro256. Your comments and suggestions are much appreciated!

As I mentioned in another thread, hipinfo shouldn't be necessary, and installing data files into libdir is counter to Fedora's FHS policy. You can discard the commit if you wish, but for strictness, if we're going to try to get this into Fedora proper, I would suggest discarding it for now.

Good point. I agree that these files should not be installed, and the information therein should be hard-coded into hipvars.pm during build time.

I was speaking to upstream and it seems like they want to merge all of the source into a single repo (hipamd, rocclr, opencl), so maybe in the longterm we can merge the hip package into Fedora's rocm-opencl, or add a new parent package and make opencl and hip subpackages.

I think that is the most sensible approach for these components. In Fedora, I believe, this should then be 1 spec file with a few sub-packages depending on how this pans out with the merging.

Last, I haven't had any issues with the permissions when building the package from the tar files on my local system. Here the essential steps for building the packages (on an up-to-date Fedora 36 system where I installed the build dependencies manually):

rpmdev-setuptree
spectool -g -R rocm-hip.spec
rpmbuild --verbose -bb --target x86_64 rocm-hip.spec

@awehrfritz awehrfritz merged commit 1415f0d into awehrfritz:main Sep 7, 2022
@Mystro256
Copy link
Contributor Author

I haven't had any issues with the permissions when building the package from the tar files on my local system

Interesting, When I used spectool, the files I got seemed to set all the permissions to 775 for the scripts... no idea why. It didn't do it before when I tried 5.1. I guess it does no harm :)

In Fedora, I believe, this should then be 1 spec file with a few sub-packages depending on how this pans out with the merging.

Yes I'll try to keep you updated. They've been emailing me with their proposals, and I think the idea of having ROCclr, hipamd, and ROCm-OpenCL in one git is fine. It's much better than the current 4 sources to build hip and reduces the bundling related concerns, since HIP uses a bunch of the openCL source to build.

information therein should be hard-coded into hipvars.pm during build time.

Regarding the hipvars.pm, I'll reach out to some people to see if there's a possibility to move it to a more traditional location for perl scripts, such as libdir/perl# or datadir/perl#. With hipinfo and hipversion gone, I think the hipvars.pm is the only complaint I have left regarding installation locations. I'll make a github issue to track if there's no response.

@cgmb
Copy link

cgmb commented Sep 8, 2022

@cgmb was working with Debian to resolve these, but he might not be available to provide input right now. He might be a better contact for hipcc related issues. I believe he works on a lot of the HIP libraries needed for applications such as pytorch.

I am a rocSOLVER and rocm-cmake maintainer, but I can directly help with pretty much any roc* or hip* library under the ROCmSoftwarePlatform organization. I volunteer with Spack and Debian to help with their packaging. I'm not really an expert on hipcc... just a heavy user who has dug into some of the details.

It's true that I'm not available much right now. My daughter is just over six weeks old, and I'm currently out on parental leave. Still, I do find bits of time here and there to answer questions or fix bugs... it's just a bit intermittent.

@Mystro256
Copy link
Contributor Author

@cgmb Thanks, I appreciate the help. As I told you before, I'm a bit lost when it comes to HIP and the dependant libraries :)

@awehrfritz I reached out to the hipcc maintainers, and it looks like there's a desire to move away from perl in the near future, so a lot of these issues should go away. I'm not 100% clear of the timeline, so I suggest we go with Debian's workarounds for now. If one of us can get it working, then I can put in a review request to import it into Fedora/EPEL.

Side note, if you have interest in becoming a Fedora packager, please let me know, as I welcome package co-maintainership :)

@cgmb
Copy link

cgmb commented Sep 8, 2022

@awehrfritz I reached out to the hipcc maintainers, and it looks like there's a desire to move away from perl in the near future, so a lot of these issues should go away.

Some of them might, but the replacement looks like it's basically a port of the perl script to C++. It will inherit many of the same troubles. I'm not clear on the timeline for upstream ROCm to switch to the new hipcc, but it's probably quite soon.

@awehrfritz
Copy link
Owner

Some of them might, but the replacement looks like it's basically a port of the perl script to C++. It will inherit many of the same troubles. I'm not clear on the timeline for upstream ROCm to switch to the new hipcc, but it's probably quite soon.

I agree with that. As long as upstream doesn't do the switch, I believe we should fix the perl scripts and let them deal with porting everything to the C++ version. I have created a PR for setting the hip version consistently without having to rely on .hipVersion (ROCm/HIP#2937).

@Mystro256, regarding becoming a Fedora packager, the time I can allocate for this is really very limited, but I will consider it for the rocm-hip package once this is working and we have it ready for inclusion in Fedora. (I still see there quite some issues.)

@Mystro256
Copy link
Contributor Author

@Mystro256, regarding becoming a Fedora packager, the time I can allocate for this is really very limited, but I will consider it for the rocm-hip package once this is working and we have it ready for inclusion in Fedora. (I still see there quite some issues.)

If you have something working, I'm more than happy to pull it into Fedora. I just don't have the ability to do functional testing with HIP. In other words, I'm happy to do the packaging work, but less able to help with more functional related issues.

OpenCL isn't too bad because it's not as changing as HIP is, since OCL has a much tighter defined specification with better/more available test suites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants