Slimbook-Team / slimbookamdcontroller

GNU General Public License v3.0
35 stars 8 forks source link

Fix pgrep length limit #31

Open niveK77pur opened 1 year ago

niveK77pur commented 1 year ago

This fix is related to https://github.com/Slimbook-Team/slimbookbattery/commit/5ec4ab95760a738b690242beb0db0171aab2f3cc.

This patch makes the program run in Debian and other systems. In them, pgrep has a limit of 15 characters, so the "slimbookbatterypreferences" string is too long for it. This patch uses the -f parameter to fix this.


I was working on a rather elaborate fix for this, since I did not know whether pgrep's -f option is available across all versions and systems. However, as I was getting ready to make a fork for a PR, I spotted the linked commit on the slimbookbattery repository to address the issue (I am having the exact same issue with that application as well). I also spot pgrep -f in other places in this repository, so I suppose the flag was simply forgotten here.

I hope I am not overlooking something, which makes the exclusion of -f intentional. Reason for this fix, is that I cannot launch the slimbookamdcontroller myself, even if it is not running (it prints to the terminal that the application is already running) — only way to launch it is to allow it at startup. After some digging, I found the current pgrep command to spit out an error, which triggers the following if conditions wrongly.

P.S. This is my first every code contribution to a project!

Lt-Henry commented 11 months ago

Hi, thanks for the patch. I am currently looking into it but adding -f option makes application complain about already loaded when It is not. I think (not the author of this code) this initialization is broken, no matter if -f or not. Later in the initialization sequence, there is another call to pgrep -f which it seems to work properly (using Popen instead of getoutput)

jcarolinares commented 2 weeks ago

I confirm what @Lt-Henry is saying. With a freshly upgrade Ubuntu 24 from version Ubuntu 22 the -f command is not enough

With Ubuntu 22 I've fixed some time days ago following: https://github.com/Slimbook-Team/slimbookamdcontroller/issues/30 and installing with PIP

pip install pyamdgpuinfo

In this case is not working and due to the changes of ubuntu 24 it is more confusing:

carolinares@jcarolinares-TITAN:~$ pip install pyamdgpuinfo
error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
    python3-xyz, where xyz is the package you are trying to
    install.

    If you wish to install a non-Debian-packaged Python package,
    create a virtual environment using python3 -m venv path/to/venv.
    Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
    sure you have python3-full installed.

    If you wish to install a non-Debian packaged Python application,
    it may be easiest to use pipx install xyz, which will manage a
    virtual environment for you. Make sure you have pipx installed.

    See /usr/share/doc/python3.12/README.venv for more information.

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.

hopefully this can bring more clues, thank you both!

EDIT: I see the AMD Controller disappeared from Ubuntu 24.04 not being possible of installing it anymore. I get it if it is not working I just wonder if you guys have plans to it soon or how can I control the Ryzen TPU via commands in the meantime, thanks!

Lt-Henry commented 2 weeks ago

I confirm what @Lt-Henry is saying. With a freshly upgrade Ubuntu 24 from version Ubuntu 22 the -f command is not enough

With Ubuntu 22 I've fixed some time days ago following: #30 and installing with PIP

pip install pyamdgpuinfo

In this case is not working and due to the changes of ubuntu 24 it is more confusing:

carolinares@jcarolinares-TITAN:~$ pip install pyamdgpuinfo
error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
    python3-xyz, where xyz is the package you are trying to
    install.

    If you wish to install a non-Debian-packaged Python package,
    create a virtual environment using python3 -m venv path/to/venv.
    Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
    sure you have python3-full installed.

    If you wish to install a non-Debian packaged Python application,
    it may be easiest to use pipx install xyz, which will manage a
    virtual environment for you. Make sure you have pipx installed.

    See /usr/share/doc/python3.12/README.venv for more information.

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.

hopefully this can bring more clues, thank you both!

EDIT: I see the AMD Controller disappeared from Ubuntu 24.04 not being possible of installing it anymore. I get it if it is not working I just wonder if you guys have plans to it soon or how can I control the Ryzen TPU via commands in the meantime, thanks!

We have refactored both amd and intel controller, cleaning up a bit the code, fixing some bugs and improving package quality a bit.

From 24.04, SlimbookOS no longer uses launchpad but its own repository. We are however, keeping launchpad for those who already configured it and are more comfortable with a vanilla Ubuntu. Give me a couple of days and I will move amdcontroller to launchpad too.