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

Use usr/local/bin instead of usr/bin for Silverblue and other immutable OSs #460

Merged
merged 2 commits into from
Nov 24, 2022
Merged

Conversation

gimbling-away
Copy link
Contributor

Fixes #394

@AdnanHodzic
Copy link
Owner

Great stuff and thank you for your contribution!

@AdnanHodzic AdnanHodzic merged commit 122dd0f into AdnanHodzic:master Nov 24, 2022
@gimbling-away
Copy link
Contributor Author

For future references: This should make it also compatible with OpenSUSE MicroOS and other immutable operating systems.

@SunkenHero
Copy link
Contributor

Great work.

@AdnanHodzic
Copy link
Owner

@gimbles while reviewing #459 I noticed a particular regression introduced with this PR.

Namely, prior to these changes if you perform an install auto-cpufreq with sudo ./auto-cpufreq-installer --install followed by installation of daemon sudo auto-cpufreq --install, then you decide to remove everything with sudo ./auto-cpufreq-installer --remove you'll get following output:

sudo ./auto-cpufreq-installer --remove 

--------------------- Removing auto-cpufreq daemon ----------------------

* Turn on bluetooth on boot

----------------------------------- Warning -----------------------------------

Detected GNOME Power Profiles daemon service is stopped!
This service will now be enabled and started again.

* Enabling GNOME power profiles
Removed "/etc/systemd/system/power-profiles-daemon.service".
Created symlink /etc/systemd/system/graphical.target.wants/power-profiles-daemon.service → /lib/systemd/system/power-profiles-daemon.service.

------------------ Running auto-cpufreq daemon removal script ------------------

* Stopping auto-cpufreq daemon (systemd) service

* Disabling auto-cpufreq daemon (systemd) at boot
Removed "/etc/systemd/system/multi-user.target.wants/auto-cpufreq.service".

* Removing auto-cpufreq daemon (systemd) unit file

* Reloading systemd manager configuration
reset failed

------------------------- auto-cpufreq daemon removed -------------------------

auto-cpufreq successfully removed.

-------------------------------------------------------------------------------


-------------------------------------------------------------------------------

auto-cpufreq tool and all its supporting files successfully removed.

-------------------------------------------------------------------------------

However, after these changes have been merged if you do the same steps as mentioned above, you'll only be presented with:

sudo ./auto-cpufreq-installer --remove 

-------------------------------------------------------------------------------

auto-cpufreq tool and all its supporting files successfully removed.

-------------------------------------------------------------------------------

I'd really like if same more verbose removal output was brought back as it gives more clarity to user on what happened. Can you please create a new PR for this? Thank you

@AdnanHodzic AdnanHodzic mentioned this pull request Nov 27, 2022
@gimbling-away
Copy link
Contributor Author

I don't really have an idea on why this regression is caused.

I didn't even touch the installer, just replaced a few paths in core.py

@AdnanHodzic
Copy link
Owner

I don't really have an idea on why this regression is caused.

I didn't even touch the installer, just replaced a few paths in core.py

@gimbles I know, I'm also baffled myself. But have tracked it down where d77c1de commit (one before these changes were merged) doesn't have this behavior.

@SunkenHero can you confirm if you have this problem as well?

@abvee
Copy link
Contributor

abvee commented Nov 27, 2022

The installer script just evaluates and runs /usr/local/bin/auto-cpufreq --remove when it tests that auto-cpufreq-remove exists.

If this test fails, then it's likely that nothing is removed and the function silently goes on to print auto-cpufreq tool and all its supporting files successfully removed.

This would also explain the lack of messages.

We should add a check in the installer to see if the test is successful or not, which would also help in debugging this particular issue. So something like this:

  if [ -f $srv_remove ]; then
    eval $tool_proc_rm
  else
  	separator
	printf "Failed to uninstall"
	separator
	return 1;
  fi

@AdnanHodzic
Copy link
Owner

We should add a check in the installer to see if the test is successful or not, which would also help in debugging this particular issue. So something like this:

  if [ -f $srv_remove ]; then
    eval $tool_proc_rm
  else
  	separator
	printf "Failed to uninstall"
	separator
	return 1;
  fi

@abvee please give it a try and needless to say create a PR if you're successful :)

@AdnanHodzic AdnanHodzic mentioned this pull request Nov 27, 2022
@abvee
Copy link
Contributor

abvee commented Nov 28, 2022

@abvee please give it a try and needless to say create a PR if you're successful :)

I have some code, but I'm a bit lost on what to do with it. It's my first time actually contributing to a repository, so I'm not sure what best practice here is..... do I make a fork of @gimbles repository or just a new branch and PR on my own fork that includes my installer changes ?

@AdnanHodzic
Copy link
Owner

@abvee you would create a PR from auto-cpufreq master branch. Changes made in this PR by @gimbles have already been merged.

@abvee
Copy link
Contributor

abvee commented Nov 28, 2022

@abvee you would create a PR from auto-cpufreq master branch. Changes made in this PR by @gimbles have already been merged.

But what about the changes I have made to the installer in #459 that have not been merged yet ? Do I modify the original install script and let you do the merging ? Sorry for all the unrelated questions, couldn't find any proper answers on google :(

@AdnanHodzic
Copy link
Owner

But what about the changes I have made to the installer in #459 that have not been merged yet ? Do I modify the original install script and let you do the merging ?

Don't worry about it, I'll merge it as soon as this regression has been fixed. Reason why I didn't merge it yet is I'm trying to reduce entropy around this issue.

AdnanHodzic pushed a commit that referenced this pull request Nov 30, 2022
* Added an error message when the file isn't found

* $srv_install and $srv_remove are now at new locations
Created _old variables to keep track of old locations

* Added exit statement and specified daemon not removed

* Fixed some errors
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.

Not installable on Fedora Silverblue 36 (35 too)
4 participants