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

Packaging rework #531

Conversation

K900
Copy link
Contributor

@K900 K900 commented Aug 25, 2023

Please tick as appropriate:

  • I have tested this code on a steam deck or on a PC
  • My changes generate no new errors/warnings
  • This is a bugfix/hotfix
  • This is a new feature

If you're wanting to update a translation or add a new one, please use the weblate page: https://weblate.werwolv.net/projects/decky/

Description

As promised in #528.

This reworks Decky's packaging to look more like a normal Python package, allowing us to use the package metadata for updates and such. The only part missing right now is actually setting the correct version in CI, which is the most interesting one, but I'm posting this as a draft to hopefully get some feedback.

@K900 K900 force-pushed the versioning branch 2 times, most recently from b2c963c to 23120d6 Compare August 25, 2023 18:07
@TrainDoctor
Copy link
Member

Looking good already @K900. We are actually trying a fix for some regressions by downgrading python to 3.10.6. I'll let you know if we go forward with this change (more than likely).

@TrainDoctor TrainDoctor requested a review from a team August 25, 2023 18:12
@TrainDoctor TrainDoctor added duplicate This issue or pull request already exists enhancement New feature or request and removed duplicate This issue or pull request already exists labels Aug 25, 2023
@K900
Copy link
Contributor Author

K900 commented Aug 25, 2023

What kind of regressions? I'd be very surprised if there are actual regressions related to Python versions and not the packaging.

@TrainDoctor
Copy link
Member

What kind of regressions? I'd be very surprised if there are actual regressions related to Python versions and not the packaging.

Plugins not working, and Decky itself misbehaving on users devices. We're trying to rule out each potential change and starting with the python backend.

@K900 K900 force-pushed the versioning branch 2 times, most recently from f7326aa to 3a11e57 Compare August 25, 2023 18:30
@K900
Copy link
Contributor Author

K900 commented Aug 25, 2023

Also this includes the changes from #528 because this undoes one of those changes.

@TrainDoctor
Copy link
Member

Also this includes the changes from #528 because this undoes one of those changes.

Probably safe to close that one then? This looks good already. Myself as leading the CI work and @SkyLeite who develops our CLI tool should both take a look at this so we can provide you the desired feedback.

@K900
Copy link
Contributor Author

K900 commented Aug 25, 2023

I'd rather get #528 merged first as those changes are tiny and this will need a bunch of tweaking and testing before we're confident in it.

@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

Also ping here: is the general approach something you're interested in? I don't really want to keep rebasing this if it's not going to land.

@TrainDoctor
Copy link
Member

Also ping here: is the general approach something you're interested in? I don't really want to keep rebasing this if it's not going to land.

Definitely interested, I'm waiting for @AAGaming00 to approve #528 before I go forward with this PR. Please let us know when you're ready to remove the draft status of this PR.

@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

I think if I rebase this, it'll be mergeable as-is, and we can then redo the CI bits to make versioning work correctly in a follow-up. Though I haven't really tested the on-device deployment scripts carefully enough, as those don't really work on NixOS anyway.

@AAGaming00
Copy link
Member

I'm also very much interested in this, will likely merge it once it works in CI

@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

Rebased on top of master, and realized this isn't actually mergeable as-is, as the tags will be versioned wrong, but I can revert that logic.

@K900 K900 marked this pull request as ready for review September 2, 2023 17:34
@K900 K900 changed the title Versioning/packaging rework (WIP) Packaging rework Sep 2, 2023
@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

OK, so this should be mergeable now. This is just packaging changes, there's a few tiny behavior changes to make it work with the new packaging, but this should be good to go.

@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

Note that this doesn't actually embed the version metadata into the archive, that will require reorganizing the CI scripts, but that can be done in a separate change.

@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

Added another very small fix to make it so the CI artifacts are usable directly, without prior setup on the system.

@K900 K900 mentioned this pull request Sep 9, 2023
4 tasks
@K900
Copy link
Contributor Author

K900 commented Sep 21, 2023

Um, anyone?

@K900 K900 force-pushed the versioning branch 2 times, most recently from e53b751 to 1ee277b Compare November 10, 2023 17:55
@marios8543
Copy link
Member

I like it. Refactoring the backend folder so it follows the structure of a python project more is definitely the right call. A few notes, decky_plugin.py, in its current form needs to have its path added as a python path so plugins can find and import it, and also have its path in the pyinstaller data add. Also some refactoring changes have been made already by AA and me, check out my branch, and there will be a few more changes with language-independent backends, coming a bit further down the road. You can dismiss the src subdir in favor of the more pythonic form you have, but if you could rebase based on my branch it would be great.

Good work at any rate!

@K900
Copy link
Contributor Author

K900 commented Nov 11, 2023

decky_plugin would end up on PYTHONPATH by default because I'm adding it to Pyinstaller's hidden_imports, so it gets copied into the final site-packages. I'll rebase in a bit.

@K900 K900 changed the base branch from main to marios8543/async-plugin-method-requests November 11, 2023 19:57
@K900
Copy link
Contributor Author

K900 commented Nov 11, 2023

Rebased, had to pick up the changes from master, otherwise there's a merge conflict between yours and cc08412

@marios8543
Copy link
Member

marios8543 commented Nov 13, 2023

Uh you probably need to change src. here to plugin_loader. or something, so plugins can import the modules. Eg steamgriddb fails with:

[sandboxed_plugin][ERROR]: Failed to start SteamGridDB!
Traceback (most recent call last):
   File "decky_loader/plugin/sandboxed_plugin.py", line 85, in initialize
   File "<frozen importlib._bootstrap_external>", line 883, in exec_module
   File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
   File "/home/deck/homebrew/plugins/decky-steamgriddb/main.py", line 11, in <module>
     from settings import SettingsManager # type: ignore
 ModuleNotFoundError: No module named 'settings'

which is consistent to how it failed when we refactored to src. before

@K900
Copy link
Contributor Author

K900 commented Nov 13, 2023

Oh, uh, yes, that is very cursed.

@marios8543
Copy link
Member

We're gonna refactor what modules plugins can access eventually, to a common stable API that's independent to decky internals, but for now we should keep it compatible with existing plugins.

K900 added 2 commits November 13, 2023 19:05
There's no need to special case it anymore, just treat it like any other Python module.
@K900
Copy link
Contributor Author

K900 commented Nov 13, 2023

Applied.

@marios8543
Copy link
Member

 [sandboxed_plugin][ERROR]: Failed to start SteamGridDB!
 Traceback (most recent call last):
   File "decky_loader/plugin/sandboxed_plugin.py", line 79, in initialize
 KeyError: 'localplatform.localplatformlinux'

fails like this now. Not sure if anything else changed structure-wise

@K900
Copy link
Contributor Author

K900 commented Nov 13, 2023

I think this one should work.

@marios8543
Copy link
Member

Yep this works great.

@marios8543
Copy link
Member

Oh you set my async request branch as base. I think I can go ahead and merge it then

@marios8543 marios8543 merged commit 5a633fd into SteamDeckHomebrew:marios8543/async-plugin-method-requests Nov 13, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants