dominiksalvet / asus-fan-control

Fan control for ASUS devices running Linux
MIT License
320 stars 37 forks source link

Switch from rc.local to systemd #10

Closed agura-lex closed 4 years ago

agura-lex commented 4 years ago

Description Switch to systemd so it's more portable and simple and we can get rid of rc.local parsing

Additional context Issue https://github.com/dominiksalvet/asus-fan-control/issues/9

agura-lex commented 4 years ago

Looks good to me, BUT I haven't tested it since I've got no ASUS machines running Ubuntu and I feel kinda lazy to spin up a VM (I can when I've got time to if it's needed though). So I advise you to review and test it carefully before merging. :)

dominiksalvet commented 4 years ago

Hello! Sorry, but I do not have much time right now. It would be great if you can test your changes in Ubuntu VM yourself. You can use GitPack to install it. Use <your-repo-url>=<desired-branch> and you should be good to go. However, first remove asus-fan-control with my URL if any.

As soon as I have time, I will look into individual changes. :smile:

agura-lex commented 4 years ago

After one fix it installs and uninstalls properly

root@ubuntu-vm:~# gitpack install github.com/agura-lex/asus-fan-control=systemd
Created symlink /etc/systemd/system/multi-user.target.wants/asus-fan-control.service → /etc/systemd/system/asus-fan-control.service.
/usr/local/bin/asus-fan-control: Missing temperatures for your notebook model or they are invalid. Falling back to UX430UA temperatures.
[install] github.com/agura-lex/asus-fan-control systemd
root@ubuntu-vm:~# systemctl start asus-fan-control.service 
root@ubuntu-vm:~# systemctl status asus-fan-control.service 
● asus-fan-control.service - Fan control for ASUS devices running Linux
   Loaded: loaded (/etc/systemd/system/asus-fan-control.service; enabled; vendor preset: enabled)
   Active: inactive (dead) since Fri 2019-12-20 12:38:15 UTC; 7s ago
  Process: 10025 ExecStart=/usr/local/bin/asus-fan-control (code=exited, status=0/SUCCESS)
 Main PID: 10025 (code=exited, status=0/SUCCESS)

Dec 20 12:38:15 ubuntu-vm systemd[1]: Starting Fan control for ASUS devices running Linux...
Dec 20 12:38:15 ubuntu-vm asus-fan-control[10025]: /usr/local/bin/asus-fan-control: Missing temperatures for your notebook m
Dec 20 12:38:15 ubuntu-vm systemd[1]: Started Fan control for ASUS devices running Linux.
root@ubuntu-vm:~# gitpack uninstall github.com/agura-lex/asus-fan-control=systemd
Removed /etc/systemd/system/multi-user.target.wants/asus-fan-control.service.
[uninstall] github.com/agura-lex/asus-fan-control systemd
root@ubuntu-vm:~# 
agura-lex commented 4 years ago

The only think I'd like to test though is removing rc.local leftovers, I'll install your latest version, then mine and check if that works as expected

agura-lex commented 4 years ago

And I realized that after uninstalling the your latest release, it'll gitpack will clean rc.local via postrm, so there's no point in testing it this way and I don't know if gitpack has any update logic

dominiksalvet commented 4 years ago

About the removing rc.local leftovers, I wanted to inform you in my previous post but I had forgotten about it. You know, GitPack is simple. Update is basically uninstallation followed by installation... xD So yeah, it is basically impossible for any rc.local leftovers to be present as long as the uninstallation is correct.

I will review this PR very soon.

agura-lex commented 4 years ago

Should be fine now

agura-lex commented 4 years ago

My bad, rushed that without going into details

dominiksalvet commented 4 years ago

Hey man, it works flawlessly!! Thank you so much! :smile: I am merging it and after the merge I am going to silence systemctl commands as I do not want them to be visible when installing using GitPack. Good job! :rocket:

dominiksalvet commented 4 years ago

Fixes #9

dominiksalvet commented 4 years ago

I am going to release a new version to provide systemd solution for all. But right know, I am thinking about the end of the line dots (asus-fan-control description, CHANGELOG items, etc.)... Any opinions?

I want to make things as simple as possible and this seems like it is matching the pattern.

agura-lex commented 4 years ago

Woot! 🎉

What's wrong with the dots? They look okay to me and https://keepachangelog.com/en/1.0.0/ seems to have 'em as well.

I don't think it's something critical though. Like it doesn't affect readability or something else. The only concern is that it's something easy to forget about (even I did despite being kinda pedantic about that in general).

dominiksalvet commented 4 years ago

All right, no dots. Also, I don't have much time right know - I'm re-uploading my old Minecraft redstone creations to Planet Minecraft... xD Feel free to take a look around if you are in.

Anyways, I will release the new version soon.

dominiksalvet commented 4 years ago

2.11.0 is out! Just use sudo gitpack install github.com/dominiksalvet/asus-fan-control if you don't have installed your fork.

PS: I just have realized that you are possibly going to install it from AUR... so no GitPack thing I guess... xD It would be also great to update the AUR repo if possible... :smile:

agura-lex commented 4 years ago

Somehow I missed your last 2 messages, but I have noticed that you released 2.11, so I've updated my AUR package and polished some rough edges, hope I'll only have to increment version numbers for future releases. xD

Yeah, not using gitpack because I prefer managing stuff with a system package manager when possible, at least when it comes to system-wide installations. :D And I also couldn't resist the opportunity to contribute stuff to AUR since writing PKGBUILDs is kinda fun, plus now Arch and Arch-based distro users can find and install it easier and in a familiar way.

BTW, the script is really great, searched for something like that quite a while ago and couldn't find a working solution until stumbling upon this repo recently. Thanks for your great effort in making it really simple for end users (and for other devs to understand, the code is really readable and straightforward)!

dominiksalvet commented 4 years ago

I completely understand your attitude about using a system package manager rather than any other. Why introduce any unnecessary fragmentation? Can I trust that package manager? Does it work as intended? Etc. :smile:

Nevertheless, GitPack has been created as a very simple Git-based package manager. It is entirely written in POSIX shell using only POSIX utilities (e.g., sed does not even have an -i option), so it runs practically anywhere. The essential idea behind it was following. Why should we distinct the development and the distribution of our projects? And why should we do that even for interpreted programs without any specific dependencies? Update a Git repository with new changes, create a new tag and boom - a new version has been released and it is immediately available through GitPack for everyone. It is basically intended to be the first considered package manager for projects due to its simplicity and quick setup.

Not only it uses the most used version control system, Git, but it also benefits from a lot of Git's features as well. E.g., delta updates are possible thanks to Git fetching mechanism (which downloads only changes) and version order (decide whether a version is newer than other version) is completely controlled by Git's internals.

I expect you meant GitPack by "the script" in your previous message. If so, I encourage you to also take a quick look at gim, GitPack's predecessor. The development of gim and GitPack has added one year to my bachelor's study... :smile: How funny if you consider that I am rather focused on processors architecture designing. Still, I needed GitPack for some stuff in this area back then and it was also the reason why I have continued with its development.

I really appreciate your kind words. Thank you so much! :heart: I am a perfectionist and I have put a lot of effort to have it at least close-to-perfect. Keeping it simple has shown to be the best approach for me. I hope you were really talking about GitPack. :smile:

Please, make sure you will take a look at the issue #11.

Wish you a Merry Christmas! :smiley:

agura-lex commented 4 years ago

I meant the "asus-fan-control" script xD But I do think GitPack is a nice idea, especially for home installations or simple system-wide stuff.