C0rn3j / sc-controller

User-mode driver and GTK3 based GUI for Steam Controller
GNU General Public License v2.0
24 stars 2 forks source link

Fix type error on Bullseye #23

Closed git-developer closed 1 month ago

git-developer commented 1 month ago

Fix for a crash caused by a change from 2850ecd:

# ./sc-controller-v0.4.8.21-1-bullseye-x86_64.AppImage daemon debug
Traceback (most recent call last):
  File "/tmp/.mount_sc-conytlXRP/usr/bin/scc-daemon", line 2, in <module>
    from scc.sccdaemon import SCCDaemon
  File "/tmp/.mount_sc-conytlXRP/usr/lib/python/scc/sccdaemon.py", line 15, in <module>
    from scc.device_monitor import create_device_monitor
  File "/tmp/.mount_sc-conytlXRP/usr/lib/python/scc/device_monitor.py", line 35, in <module>
    class DeviceMonitor(Monitor):
  File "/tmp/.mount_sc-conytlXRP/usr/lib/python/scc/device_monitor.py", line 196, in DeviceMonitor
    def get_vendor_product(self, syspath: str, subsystem: str | None = None) -> Tuple[int, int]:
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
C0rn3j commented 1 month ago

I will jump out of my skinsuit, this will work only on 3.10+, but my linter setup does not catch it.

Please change this to double quote the types instead - "str | None", we do want them.
It will work thanks to annotations being imported.

I believe I have littered the code with this notation at this point though, so this will not be the only example :/

Fun read for bored souls - https://github.com/fastapi/typer/issues/371

git-developer commented 1 month ago

I changed it as supposed.

The AppImage builds use environments matching the base OS, so e.g. of the Focal build uses Python 3.8. Maybe it's possible to add some checks to the ci build?

C0rn3j commented 1 month ago

The problem with implementing checks for it is that I am already trying to do so with Ruff.
pylint scc/device_monitor.py --py-version '3.8' | grep -v 'Bad indentation' does not catch it either.
I might have to report an issue on the Ruff tracker about it, it looks like a bug.

git-developer commented 1 month ago

OK, I see. Once it sorted, do you think it helpful to add pylint to the ci build?

C0rn3j commented 1 month ago

I don't think it'd be too helpful at the moment, as even with the Ruff rules we have right now, the entire codebase lights up like a Christmas tree.

I think it would be helpful to define more specific Ruff rules as the codebase gets into a better shape, as I have been doing so far.

Just to confirm, Debian bullseye which this breaks runs on 3.9, not 3.8, right?

C0rn3j commented 1 month ago

Oh, and just to triple confirm, you were not testing on that specific commit were you?

As I have only added from __future__ import annotations in a later commit to that file.

git-developer commented 1 month ago

Just to confirm, Debian bullseye which this breaks runs on 3.9, not 3.8, right?

Yes, Focal has Python 3.8 and Bullseye has 3.9. But I didn't try with Focal, it might be affected, too.

git-developer commented 1 month ago

Oh, and just to triple confirm, you were not testing on that specific commit were you?

I tested release sc-controller-v0.4.8.21-1-bullseye-x86_64.AppImage as shown above.

git-developer commented 1 month ago

Just tried with sc-controller-v0.4.9.0.1-bullseye-x86_64.AppImage but can't tell because https://github.com/C0rn3j/sc-controller/issues/24 is in the way.

git-developer commented 1 month ago

But I didn't try with Focal, it might be affected, too.

Focal is affected, too. Jammy (Python 3.10) works.

C0rn3j commented 1 month ago

3.10 working makes sense, as it's string by default even without the annotations import, let's deal with the enum insanity in #24 first, test that this is still a problem on a new build that definitely has the future import in it and if so let's pull it.

C0rn3j commented 1 month ago

Please retest on https://github.com/C0rn3j/sc-controller/releases/tag/v0.4.9.1 (CI job: https://github.com/C0rn3j/sc-controller/actions/runs/10974376216) to confirm whether this is actually needed.

EDIT: Actually am pretty sure this should be broken on the AppImages despite CI passing, ioctl-opt is now separated out and needs to be installed via pip on pretty much every distribution.
I packaged it on Arch as aur/python-ioctl-opt but that means it'll still be easier to just use pip for the CI builds.

EDIT2: Yup, would that be just adding something akin to this in the global AppImageBuilder.yml

    # install python3-ioctl-opt manually as it is not packaged anywhere
     pip install --target "${TARGET_APPDIR}/usr/lib/python3/dist-packages/" ioctl-opt

and an addition in scc-linux.yml?

git-developer commented 1 month ago

I created a separate https://github.com/C0rn3j/sc-controller/pull/28 to keep things separated.

git-developer commented 1 month ago

The type error does not occur in sc-controller-v0.4.9.1.1-bullseye-x86_64.AppImage, so this PR can be closed.

Unfortunately, the icons are missing, too, but that's another story.

C0rn3j commented 1 month ago

Thanks for testing!

git-developer commented 1 month ago

You're welcome!

I saw that you're adding a BETA suffix to the current releases. When you enter the release page of a GitHub release, you can tick the checkbox Set as a pre-release on the bottom of the page.

C0rn3j commented 1 month ago

I know, I wanted the beta to be more visible as testers would be really welcome with how much breakage it has.

Regarding the icons, there's at least one bug in UDataManager that's trying to check for a nonexistent controller-icons, but that might as well be harmless:

W UDataManager  enumerate_children_finish for /home/user/.config/scc/controller-icons failed: g-io-error-quark: Error opening directory '/home/user/.config/scc/controller-icons': No such file or directory (1)
git-developer commented 1 month ago

False alarm, icons are OK. It's my fault. For icon tests, I've started the AppImage with a clean environment (via env -i DISPLAY=:0). That's not clean enough apparently, because $HOME/.config/gtk-3.0/settings.ini is in effect (even if HOME is not set). Icons are shown properly when the AppImage is started using either of

Regarding controller-icons: Looking at the code, this is a directory that is bundled with the application (usr/share/scc/images/controller-icons) and may be overridden by the user in $XDG_CONFIG_HOME/scc. Maybe the application should create an empty directory? Anyway, I think the warning may be ignored.