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

Update feature added #526

Closed
wants to merge 15 commits into from
Closed

Update feature added #526

wants to merge 15 commits into from

Conversation

cosmos1721
Copy link
Contributor

@cosmos1721 cosmos1721 commented Jun 20, 2023

As discussed in issue #341 the commit changes are hereby elaborately described:

  • update using sudo ./auto-cpufreq-installer and sudo auto-cpufreq --update
  • version number will update automatically everytime the package is updated to the latest release
  • some functions that have not been defined (showing warnings) have been commented and bypassed
  • when updated using installer:
    -- it removes the package and clones with latest release
  • when updated using --update mode, the package can be found in $HOME directory
  • README.md updated

Please let me know if changes are required.

@AdnanHodzic
Copy link
Owner

On first look these changes look good! But I would like to spend some more time in review and just testing how well it works under different scenarios.

In meantime, there's a conflict with README now. Let's resolve this conflict and and since 1.9.8 version is being mention let's set it to 1.9.9 as these changes will be part of the new release.

@cosmos1721 cosmos1721 mentioned this pull request Jun 24, 2023
@cosmos1721
Copy link
Contributor Author

Although i have tried my best to find bugs and work on it,
but YES you are right, please let me know if there's anything I need to work with.
There were some functions in if-else cases that weren't defined or might have been commented down by some developer for some reason. can you please check on that, in case it affects the overall functioning?

README.md Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Jun 25, 2023

1: Actually, after thinking about this, let's put this back to 1.9.8 as part of README. As right after these changes are merged with master I won't be making an immediate release, although these changes will be announced as part of 1.9.9 release. As having a release doesn't impact any changes made here, and if there are new commits to the branch, update feature should fetch them for whoever installed using auto-cpufreq-installer.

2: Can you confirm that following this logic, only new releases will be "caught", and not any new git commit? As I think this could be really useful for people running auto-cpufreq installed using auto-cpufreq-installer, as there are periods where I don't make a release and there are a lot of changes (commits) that are made to master branch in meantime.

There were some functions in if-else cases that weren't defined or might have been commented down by some developer for some reason. can you please check on that, in case it affects the overall functioning?

3: Sorry, I didn't understand this part?

4: I also left comments/questions in rest of the pull request which I would like to address.

5: However, one big thing that's missing here is that --update functionality will not work if package is installed using snap or AUR package (which will be updated automatically by OS). Hence, what I suggest you do is catch this case with something like this, so in this case it would return something like:

sudo auto-cpufreq --update
Updates are available,
Current version: v1.9.8
Latest version: v1.9.8

Detected auto-cpufreq was installed using snap or AUR package, please update using package managers, i.e: `sudo snap refresh auto-cpufreq`.  

As without this currently it returns something like this, and similar case will be with AUR probably as it's mixing up package install with source install:

sudo auto-cpufreq --update
Updates are available,
Current version: v
Latest version: v1.9.8
Note that your previous custom settings might be erased with the following update
Do you want to update auto-cpufreq to the latest release? [y/n]: y

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

Unable to detect state of GNOME Power Profiles daemon service!
Now it's recommended to enable this service.

Steps to perform this action using auto-cpufreq: power_helper script:
git clone https://github.com/AdnanHodzic/auto-cpufreq.git
cd auto-cpufreq/auto_cpufreq
python3 power_helper.py --gnome_power_enable

Reference: https://github.com/AdnanHodzic/auto-cpufreq#configuring-auto-cpufreq

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

auto-cpufreq successfully removed.

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

/home/ahodzic
Cloning the latest release to the home directory:  
/home/ahodzic
Traceback (most recent call last):
  File "/snap/auto-cpufreq/x1/bin/auto-cpufreq", line 254, in <module>
    main()
  File "/snap/auto-cpufreq/x1/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/snap/auto-cpufreq/x1/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/snap/auto-cpufreq/x1/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/snap/auto-cpufreq/x1/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/snap/auto-cpufreq/x1/bin/auto-cpufreq", line 244, in main
    new_update()
  File "/home/ahodzic/code/auto-cpufreq/auto_cpufreq/core.py", line 194, in new_update
    run(["git", "clone", "https://github.com/AdnanHodzic/auto-cpufreq.git"])
  File "/usr/lib/python3.10/subprocess.py", line 501, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/lib/python3.10/subprocess.py", line 969, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.10/subprocess.py", line 1845, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'git'

@cosmos1721
Copy link
Contributor Author

  1. as mentioned above in conversation related to snap *beta, since I'll revert back to those changes

  2. the snippet works like this

  3. please refer to line 453 in my merge i.e. 'gnome_power_svc_disable_performance' in core.py line 88 in bin/auto-cpufreq i.e. 'daemon_not_found()' like these which weren't affecting the code but showing warning signs "NotDefined'

  4. I'll surely look into it

  5. thanks for your insights, since I was unable to understand how snapcarft and the files related to AUR worked. I'll make necessary changes and push it in some time for sure.

@AdnanHodzic
Copy link
Owner

  1. as mentioned above in conversation related to snap *beta, since I'll revert back to those changes

Sounds good.

  1. the snippet works like this

Thanks for confirming, that's how I also interpreted the changes. Regardless, I still this is great functionality that's missing, regarding getting alerted on every new commit might not be of interest to everyone anyways as most of those are in "dev" state until actual release is made.

  1. please refer to line 453 in my merge i.e. 'gnome_power_svc_disable_performance' in core.py line 88 in bin/auto-cpufreq i.e. 'daemon_not_found()' like these which weren't affecting the code but showing warning signs "NotDefined'

What do you do exactly to replicate this? As I didn't come across this during my testing.

  1. I'll surely look into it
  2. thanks for your insights, since I was unable to understand how snapcarft and the files related to AUR worked. I'll make necessary changes and push it in some time for sure.

Great, looking forward to you updates!

@AdnanHodzic
Copy link
Owner

@cosmos1721 in meantime, please ping me if there are updates as I sometimes miss certain notifications and definitely wouldn't miss anything on this one here :)

@cosmos1721
Copy link
Contributor Author

Yes there are updates but due to some personal reasons, i was unable to continue till now . Will be started soon.
I'll let you know as soon as it completes.
By the way, can you tell me how do I test the project in AUR env without booting up my machine with it

@AdnanHodzic
Copy link
Owner

Yes there are updates but due to some personal reasons, i was unable to continue till now . Will be started soon. I'll let you know as soon as it completes.

No worries take your time!

By the way, can you tell me how do I test the project in AUR env without booting up my machine with it

Regarding Snap, it's very simple, you could literally add if os.getenv("PKG_MARKER") == "SNAP": and that will do it (more examples).

Regarding AUR, I don't use it or maintain it so I don't know the answer to that.

It could definitely be done based on how auto-cpufreq-installer works and based on paths, but it's bit more complicated. Hence, what I propose is just do it for Snap for now, then when someone tries using update flag with AUR and reports an issue, we can ask him/her to help us debug it and then we can extend it for AUR in future.

@cosmos1721
Copy link
Contributor Author

Regarding AUR, I don't use it or maintain it so I don't know the answer to that.

It could definitely be done based on how auto-cpufreq-installer works and based on paths, but it's bit more complicated. Hence, what I propose is just do it for Snap for now, then when someone tries using update flag with AUR and reports an issue, we can ask him/her to help us debug it and then we can extend it for AUR in future.

okay
but still let me try if I can work on it,
I've an idea: I can run a container and then work on it. i know its quite hectic but it works till I get any alternative

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Jun 30, 2023

but still let me try if I can work on it,
I've an idea: I can run a container and then work on it. i know its quite hectic but it works till I get any alternative

Love the enthusiasms and desire to do things right :)

I've an idea: I can run a container and then work on it. i know its quite hectic but it works till I get any alternative

If you want to go down this path, container would be an option or you could also spin up Arch based distro in a VM with i.e VirtualBox

@cosmos1721
Copy link
Contributor Author

but still let me try if I can work on it,
I've an idea: I can run a container and then work on it. i know its quite hectic but it works till I get any alternative

Love the enthusiasms and desire to do things right :)

I've an idea: I can run a container and then work on it. i know its quite hectic but it works till I get any alternative

If you want to go down this path, container would be an option or you could also spin up Arch based distro in a VM with i.e VirtualBox

thanks

If you want to go down this path, container would be an option or you could also spin up Arch based distro in a VM with i.e VirtualBox

will see which one is easier to work with without consuming necessary resources

@cosmos1721
Copy link
Contributor Author

@cosmos1721 in meantime, please ping me if there are updates as I sometimes miss certain notifications and definitely wouldn't miss anything on this one here :)

certain updates have been made, although ive made changes as per the requirements, but i was unable to perform 'snapd' and aur based tests. can you please run the tests and can you please guide me though the process (if there are any bugs)

@AdnanHodzic
Copy link
Owner

@cosmos1721 I made some comments, it all lgtm, except the package manager part snap and aur which is still broken.

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Jul 5, 2023

@cosmos1721 I made some comments, it all lgtm, except the package manager part snap and aur which is still broken.

Because comments are sometimes hard to follow, what remains to be done is:

  1. Snap package will still continue with update and will output errors, if os.getenv("PKG_MARKER") == "SNAP": didn't catch it while it definitely should, maybe move it one level higher? Related.
  2. Try the check suggestion I made for AUR. (this is not as important as snap, since AUR is not maintained by me)
  3. Update the setup.py (mentioned in 1.

@AdnanHodzic
Copy link
Owner

I finally got time to take a proper look and I see what you did wrong here, you were calling verify_update() which should also have its own if os.getenv("PKG_MARKER") == "SNAP":.

Hence, instead of:

elif update:
  verify_update()
  ans = input ("Do you want to update auto-cpufreq to the latest release? [y/n]: ")
  valid_options = ['y', 'Y', 'yes', 'YES', 'Yes']
  if ans.lower() in valid_options:
      root_check()
      if os.getenv("PKG_MARKER") == "SNAP":
          print ("Detected auto-cpufreq was installed using snap")
          run("snap refresh auto-cpufreq")
          print("please update using package managers next time, i.e: `sudo snap refresh auto-cpufreq`.")
...

Please correct this same code block to look like this:

elif update:
    root_check()
    if os.getenv("PKG_MARKER") == "SNAP":
        print ("Detected auto-cpufreq was installed using snap package.")
        print("Please update by running: `sudo snap refresh auto-cpufreq`")
    else:
        verify_update()
        ans = input ("Do you want to update auto-cpufreq to the latest release? [y/n]: ")
        valid_options = ['y', 'Y', 'yes', 'YES', 'Yes']
        if ans.lower() in valid_options:
...

@cosmos1721 cosmos1721 reopened this Jul 12, 2023
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -39,11 +41,15 @@ parts:
snapdaemon.sh: usr/bin/snapdaemon

plugs:
etc-auto-cpufreq-conf:
network:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this part? Because now snap fails to build and I can't even upload the new version to beta channel due to:

Issues while processing snap:g                                                                                                                                       
- unknown attribute 'default' for interface 'network' (plugs)     

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was required for giving network/internet access to the env.
it is essential as its a basic necessity for the update to work
though the major issue is due to line 45 i.e. default: true which isn't always supported
allow-auto-connection: true should work
although I'm still figuring out some unexpected errors for lxd
I'm about make a commit, pls check if it still fails
as I was able to build using
sudo snapcraft --destructive-mode --enable-experimental-extensions
which builds directly on the host system without using containers which I believe is good for testing

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@cosmos1721 cosmos1721 closed this Jul 13, 2023
@cosmos1721 cosmos1721 deleted the main branch July 13, 2023 17:54
@cosmos1721 cosmos1721 mentioned this pull request Jul 13, 2023
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.

None yet

2 participants