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.

Sign up for GitHub

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

Jump to bottom

Merge work from other contributors #4

Open
RichardFevrier opened this issue Nov 14, 2022 · 14 comments
Open

Merge work from other contributors #4

RichardFevrier opened this issue Nov 14, 2022 · 14 comments
Assignees
@frankcrawford
Labels
enhancement New feature or request

Comments

@RichardFevrier
Copy link

Thank you for all the work you have already made 🎉

I was wondering if you would consider merging the work from @cooper151288 and @WinkelCode since they both produced a lot of work too 🙏
@WinkelCode
Copy link

WinkelCode commented Nov 14, 2022 •
edited
Loading

Hi,

I'm the person who made the RPM/akmod and AKMS (Alpine Linux) scripts (https://github.com/WinkelCode/it87)

I did the project in early 2022 to get it to work on Fedora Silverblue (which it does last time I checked), and also a bunch of additional scripting, which may or may not be unnecessarily complicated.

Basically the idea of my scripts is to make installation of the module easy without actually having it in the distro's repos, which I don't know if there is enough interest in.

My development on these scripts kind of stalled in a weird spot, I have this stupidly over the top script for RPM, and this lighter one for Alpine. Before I stopped working in it, I was looking into "synchronizing" both (maybe the DKMS one too) scripts to be more similar, possibly just merging them into one. I was also looking into refactoring the documentation (the README mainly) to be more approachable.

I'll take this issue as a sign to finally finish the project and submit a pull request here once I'm done (changes to documentation optional). My script for RPM actually doesn't need to be alongside the source files, it can grab them from a given repository, so I could alternatively split it off entirely.

@RichardFevrier
Copy link
Author

Hi @WinkelCode,

Thank you for your detailed answer!

My daily driver is Fedora Silverblue this is the reason that I found all the work you have done.
I also found this repo where I think @frankcrawford tries to stay up to date and in conformity with the official version.

Trying to regroup all of your efforts, that is the reason that I have opened this issue in the first place 🙂

@frankcrawford
Copy link
Owner

frankcrawford commented Nov 15, 2022 via email

@frankcrawford
Copy link
Owner

@RichardFevrier
Okay, I'm back from holidays now, and can spend a bit of time looking at your suggestions, and I'm fine with it, although obviously need the assistance of the others.

Firstly @WinkelCode I'm fine to add you mods in, especially as it looks like your changes are slightly orthogonal to mine, in that I haven't really spent much time on it, and happy to get something better in for it. So, I guess what I am saying is raise a PR and I'll be happy to merge it in.

As for the code from @cooper151288 I'd be happy to merge any changes, but I don't see much aside from the README file. If there is any other practical changes let me know.

@cooper151288
Copy link

cooper151288 commented Nov 21, 2022 via email

@frankcrawford
Copy link
Owner

@cooper151288 for the moment, I may leave it aside, as I try to get the drive updated in the current kernel with all the existing changes.

However, I am interested in getting it added in a future version, so I may check what changes are required.

Thanks.

@frankcrawford
Copy link
Owner

@WinkelCode just out of interest, has there been any updates on your AKMS work?

@WinkelCode
Copy link

@WinkelCode just out of interest, has there been any updates on your AKMS work?

Sorry, stuff got in the way and then I got kind of sidetracked. I'm back working on it as I'm writing this.

@WinkelCode
Copy link

WinkelCode commented Jan 31, 2023 •
edited
Loading

@frankcrawford Would you say that there is a good possibility that this out-of-tree module will still be useful in a few months? I've been thinking about just straight up writing the CI/CD stuff to automatically build and put RPM/DEB/APK packages in the repo's releases.

Also, my scripts currently default to creating these entries:

# /etc/depmod.d/it87.conf
# Slightly different for AKMS, etc.
override it87 * kernel/extra

# /etc/modules-load.d/it87.conf
it87

# /etc/modprobe.d/it87.conf
options it87 ignore_resource_conflict

Especially the ignore_resource_conflict is needed on my Gigabyte B550 Vision D's IT8688E chip, is that a good default to have, or should we handle it differently? I guess I could make it build two different packages, or an extra package?

Edit 2: I've pretty much decided I'm going to pursue the CI route. However, the question whether ignore_resource_conflict should be a default is still open, I am not sure how problematic it could be? I could just make an extra package that simply enables it, what do you guys think?

@frankcrawford
Copy link
Owner

@WinkelCode yes, I plan to keep this going for a while, and even is I update the in-tree module, this will always have a few items not accepted there.

As for ignore_resource_conflict it is pretty much essential for all recent Gigabyte motherboards. If you do some simple tests for me, I can add it into the DMI match so it is automatically set (something that will never be in the in-tree version).

Technically, we should not just set it, as there are cases where ignoring ACPI conflicts can cause issues, but I'm pretty sure that won't happen with this particular one, as Gigabyte have done nothing with it.

@WinkelCode
Copy link

Good news: I am pretty much done. Unfortunately it took much longer than I had hoped, mostly because I kept running into incredibly annoying and obscure issues which kept killing my motivation, but I managed to either fix or work around most of them.

Quick rundown:

  • packagetool.sh: Builds apk, rpm and deb packages in a container, then makes the respective dynamic module builder build them, checks if they were built (modprobeing the file), then uninstalls the package to make sure the module is properly removed (I added that one because Alpine's apk wouldn't tell AKMS to uninstall it).
  • Reusable GitHub Actions workflow for packagetool with build caching.
  • Spec files for RPM, APKBUILD and AKMBUILD for Alpine. Alpine packages are extracted to additionally deliver package-less files for manual installation in build artifact (no such capability with akmods).
  • Alpine and RPM use the following structure: Userland package with depmod (override built in module) and modload (always load module) configuration, package for the dynamic build system (AKMS/akmods), and a ignore_resource_conflict package with the self-explanatory modprobe entry. Alpine also has a separate 'doc' package as is convention. I made sure to always also include the LICENSE file along the source code (I added the GPLv2 license to the repo, as was already specified in the source code).
  • Debian is unchanged as I see it is already being maintained, might benefit from some config files from the userland packages, but I am done with learning new package systems for now 😅
  • Added an option to the Makefile to specify the module version as an argument

Notes:

  • I am not 100% sure on the compliance with conventions regarding the RPM and Alpine packaging, but it's not going into the repos so it doesn't matter too much.
  • The tool handles the current commit and current date as required, but i haven't implemented anything for "proper" versions. I am under the impression that while two tags exist, this fork was never properly versioned.
  • The kernel module gets the same version format as the package.
  • Podman should "just work" with root permissions (the module building during testing usually needs special privileges).
  • Docker needs to have the buildx thingy enabled so it can use the type of caching we need for CI. I could add an option so it also works without buildx, but supposedly that's depreciated and it doesn't support caching (idk what's going on over there exactly).
  • Fedora Immutable users should be able to just clone the repo, run packagetool.sh --container_runtime=podman --package_system=rpm and grab the .rpms from the .release directory.

Right now I am double checking stuff and finishing up, I'll have to test the packages on my bare metal machine, and make sure the package build files can be correctly used without the packagetool.sh wrapper.

@frankcrawford
Copy link
Owner

@WinkelCode

Thanks for that. Once you are happy, lets roll it into this repo.

As you noted, it is unlikely anything here will be picked up directly by any of the distributions, so something that works and is close to their standards is good enough.

I'll probably run up an Alpine VM sometime just to make sure it installs, but won't have any hardware running Alpine to test, so I am relying on you (and the wider community) to note any issues.

And again, thanks for the work.

Regards
Frank

@WinkelCode WinkelCode mentioned this issue May 7, 2023
Draft
@WinkelCode
Copy link

I created a draft pull request here: #9

Packaging is done but I need help making sure the README is up-to-date with the current status of the module.

@frankcrawford frankcrawford self-assigned this Jan 3, 2024
@frankcrawford
Copy link
Owner

@WinkelCode how is this going? I haven't really looked at it lately, but it seems to not have changed since May.

@frankcrawford frankcrawford added the enhancement New feature or request label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@frankcrawford frankcrawford

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@RichardFevrier @frankcrawford @cooper151288 @WinkelCode