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

Fix for Regression in #460 #462

Merged
merged 4 commits into from
Nov 30, 2022
Merged

Fix for Regression in #460 #462

merged 4 commits into from
Nov 30, 2022

Conversation

abvee
Copy link
Contributor

@abvee abvee commented Nov 29, 2022

The uninstall script was checking old locations (/usr/bin/<file>) that were changed by @gimbles

When the check to see if /usr/bin/auto-cpufreq-remove existed failed (since it's now at /usr/local/bin/auto-cpufreq-remove), the removal didn't occur fully and the installer silently goes on until the success message is wrongly printed.

Just adding a check for this should make it work now

However, people who downloaded older versions of auto-cpufreq will still have those scripts at the old locations (in /usr/bin), so I added some variables with an _old suffix so people cloning a new repo and running the script can still uninstall their old versions of auto-cpufreq.

We could also use /usr/local/bin for only Silverblue, which is something I can work on later, but this should fix the script for now.

@abvee abvee changed the title Regression in #460 Fix for Regression in #460 Nov 29, 2022
@SunkenHero
Copy link
Contributor

I quickly looked over it and it looks like everything should work.

@AdnanHodzic
Copy link
Owner

The uninstall script was checking old locations (/usr/bin/<file>) that were changed by @gimbles

When the check to see if /usr/bin/auto-cpufreq-remove existed failed (since it's now at /usr/local/bin/auto-cpufreq-remove), the removal didn't occur fully and the installer silently goes on until the success message is wrongly printed.

Just adding a check for this should make it work now

However, people who downloaded older versions of auto-cpufreq will still have those scripts at the old locations (in /usr/bin), so I added some variables with an _old suffix so people cloning a new repo and running the script can still uninstall their old versions of auto-cpufreq.

Great thinking!

We could also use /usr/local/bin for only Silverblue, which is something I can work on later, but this should fix the script for now.

Yea, for now it's fine and I don't mind keeping it under /usr/local/bin, as it would be good to have paths as unified as possible.

Regarding your commit, you have a syntax error, please change line 192 to from:

  if [ -f $srv_remove -o -f $srv_remove_old]; then

to:

  if [ -f $srv_remove -o -f $srv_remove_old ]; then

I also get following error when I try to remove (older then this PR) auto-cpufreq installed from current master branch:

sudo ./auto-cpufreq-installer --remove

-------------------------------------------------------------------------------
Couldn't remove the auto-cpufreq daemon
/usr/bin/auto-cpufreq-remove or /usr/bin/auto-cpufreq-remove do not exist.

and I'll run into some issue as part of your branch abvee-460-regression (even if prior to --remove I do sudo ./auto-cpufreq-installer --remove) whether auto-cpufreq daemon is installed or not ...

@abvee
Copy link
Contributor Author

abvee commented Nov 30, 2022

Regarding your commit, you have a syntax error, please change line 192 to from:

My bad, I'll fix it

I also get following error when I try to remove (older then this PR) auto-cpufreq installed from current master branch:

Like the idiot I am, I forgot to add local to the path. I just tested it, it should work now

@AdnanHodzic
Copy link
Owner

Perfect, and thank you for your contribution!

@AdnanHodzic AdnanHodzic merged commit 13edca0 into AdnanHodzic:master Nov 30, 2022
@AdnanHodzic AdnanHodzic mentioned this pull request Nov 30, 2022
@abvee
Copy link
Contributor Author

abvee commented Nov 30, 2022

Perfect, and thank you for your contribution!

Thanks :D

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