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

Install script fixes - better support for fresh installs of apt-based distros. #186

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

KrisDevelopment
Copy link

Check and enable kernel module - platform_profile on fresh installs.
Also attempts to auto-install make if not present.

@LarryTheMagicDragon
Copy link
Contributor

I've looked over the changes you've made, and there some things I'd like to bring up.

  1. What is the point of the .keyboard_cache file?
  2. I appreciate the added functionality of checking for the required dependencies, but I have concerns about the efficacy of supporting only one operating system. I feel with only marginally more effort, you could get the same level of support for all the operating systems that use the apt package manager.
  3. while these checks are run in the install script, there's nothing for the install_service script. I think it would be better if there was a script specifically for checking/resolving dependencies, this way it can be run before either script.
  4. you don't need to use sudo in the install script, lines 3-6 check to see if your root, and exit if you're not. The use of sudo is redundant, and breaks functionality for systems that don't have it installed.

@KrisDevelopment
Copy link
Author

KrisDevelopment commented Oct 12, 2024

I've looked over the changes you've made, and there some things I'd like to bring up.

  1. What is the point of the .keyboard_cache file?
  2. I appreciate the added functionality of checking for the required dependencies, but I have concerns about the efficacy of supporting only one operating system. I feel with only marginally more effort, you could get the same level of support for all the operating systems that use the apt package manager.
  3. while these checks are run in the install script, there's nothing for the install_service script. I think it would be better if there was a script specifically for checking/resolving dependencies, this way it can be run before either script.
  4. you don't need to use sudo in the install script, lines 3-6 check to see if your root, and exit if you're not. The use of sudo is redundant, and breaks functionality for systems that don't have it installed.

Yes, you're bringing up valid points. The keyboard thing seems to have slipped through due to issues with the .gitignore. The thing about effort on other platforms is valid too but I'm solving issues related to my own install, whereas I can't test it on others. Finally on the separate checks script, that would be a good idea, but the added untested failure points is not worth it for me. More moving parts means more ways to break something. Yes sudo is redundant, will see what i can do about it. Cheers.

@KrisDevelopment KrisDevelopment marked this pull request as draft October 12, 2024 23:22
@KrisDevelopment KrisDevelopment marked this pull request as ready for review October 13, 2024 10:03
@KrisDevelopment
Copy link
Author

I overlooked && exit 1 fails after the modinfo check. Should work better now.

@mmsaeed509
Copy link
Collaborator

This PR is for Debian-based distros only, so we don't have to merge it if it is missing the other distros

@LarryTheMagicDragon
Copy link
Contributor

So far it's only limitation is that it only supports the apt package manager. In principle I think what's being done here is a good idea. How many/which package managers need to be supported for this to be merged?

@mmsaeed509
Copy link
Collaborator

We don't need to write a script to check dependencies since there are various package managers. Instead, we can create binary packages for different package managers, such as pacman. I have already made a binary package for Arch-based systems, as I build an Arch-based distro called Exodia OS. Exodia OS also has a special edition for Acer Predator laptops, which comes with this kernel module pre-installed as a binary package.

You can find the binary package here: Predator-Sense-CLI-8.0-1-any.pkg.tar.zst.

Instructions on how to build the binary package can be found here.

visit Exodia OS

@LarryTheMagicDragon
Copy link
Contributor

Being that you are the developer of Exodia OS, I can see how creating a package for package managers is viable option for you. Unless we want to put the extra effort into creating packages every time there is an update for distros that use zypper, dnf, apt, pacman, and all the other package managers I'm forgetting, plus getting them added to the repos for those distros, I'm not sure this is sustainable. As an arch user I don't care if there are special features for debian based distros, the most important thing is we have functionality for as many as possible, without being a detriment to anyone. If apt users get to have make installed for free without needing to deal with it themselves, I don't see what the issue is. If I want that functionality I can add it myself.

@KrisDevelopment KrisDevelopment changed the title Install script fixes - better support for fresh installs of Debian. Install script fixes - better support for fresh installs of apt-based distros. Dec 1, 2024
@KrisDevelopment
Copy link
Author

Fixed merge issues

@LarryTheMagicDragon
Copy link
Contributor

Good to see this branch is still in development. When I made fixes for clang kernels I was using a live usb to try and test the changes I made. This would have saved me a headache

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.

3 participants