-
Notifications
You must be signed in to change notification settings - Fork 3
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 type error on Bullseye #23
Conversation
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 - 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 - fastapi/typer#371 |
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? |
The problem with implementing checks for it is that I am already trying to do so with Ruff. |
OK, I see. Once it sorted, do you think it helpful to add |
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? |
Oh, and just to triple confirm, you were not testing on that specific commit were you? As I have only added |
Yes, Focal has Python 3.8 and Bullseye has 3.9. But I didn't try with Focal, it might be affected, too. |
I tested release |
Just tried with |
Focal is affected, too. Jammy (Python 3.10) works. |
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. |
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, 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? |
I created a separate #28 to keep things separated. |
The type error does not occur in Unfortunately, the icons are missing, too, but that's another story. |
Thanks for testing! |
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. |
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
|
False alarm, icons are OK. It's my fault. For icon tests, I've started the AppImage with a clean environment (via
Regarding |
Fix for a crash caused by a change from 2850ecd: