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

Some fixes for NixOS #528

Closed
wants to merge 2 commits into from
Closed

Some fixes for NixOS #528

wants to merge 2 commits into from

Conversation

K900
Copy link
Contributor

@K900 K900 commented Aug 22, 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

Description

Probably better viewed commit by commit. To be honest, the Python codebase isn't in the best state, and I'd be happy to help clean it up, but I wanted to get the minimal set of changes actually useful for NixOS first.

Copy link
Member

@AAGaming00 AAGaming00 left a comment

Choose a reason for hiding this comment

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

please do not unminify that js, the more js you send to eval the higher chance steam will crash for no reason when reloading in my experience (in addition to it slightly delaying load which isn't ideal), that's why I minified it by hand to reduce whitespace but keep readability

also, a cache key is not needed, decky disables caching for it properly using a header here

return web.FileResponse(file, headers={"Cache-Control": "no-cache"})

@AAGaming00
Copy link
Member

as for cleaning up the py, I'm working on adding static types everywhere to make it more maintainable, and switching communication between frontend and backend to websocket

@AAGaming00
Copy link
Member

Also you may want to consider some way of disabling Decky's built in updater if you want to update it via packages

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

please do not unminify that js, the more js you send to eval the higher chance steam will crash for no reason when reloading in my experience (in addition to it slightly delaying load which isn't ideal), that's why I minified it by hand to reduce whitespace but keep readability

Maybe we can rework it to be in an external file that's minified at runtime? (or even at build time - I'm not sure if there's any decent tool for JS minification you can easily drive from Python).

I can inline the change for now, though.

also, a cache key is not needed, decky disables caching for it properly using a header here

A cache key is unfortunately needed, because aiohttp is trying to be clever and handle If-Modified-Since, and we set all the file mtimes to epoch 0 during build for reproducibility, so Steam always hits a 304 Not Modified. I couldn't find a better way to disable this behavior (and I have a good amount of experience gazing into the aiohttp abyss), except by lying to Steam.

as for cleaning up the py, I'm working on adding static types everywhere to make it more maintainable, and switching communication between frontend and backend to websocket

A few things I'd like to do that jumped out at me:

  • replace all the os.path usage with pathlib
  • encode the version of the loader into the binary itself instead of putting it in an external file
  • probably add some sort of linter+formatter to run in CI to catch things like unused imports and inaccurate type hints

Also you may want to consider some way of disabling Decky's built in updater if you want to update it via packages

I want to look into that in the future, yes.

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

Here's what the HTTP exchange looks like on my Deck running NixOS:

Lots of text
GET /frontend/index.js HTTP/1.1
Host: localhost:1337
Connection: keep-alive
Accept: */*
Origin: https://steamloopback.host
User-Agent: Mozilla/5.0 (X11; Linux x86_64; Valve Steam Client/default/1692390949) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
Sec-Fetch-Site: cross-site
Sec-Fetch-Mode: cors
Sec-Fetch-Dest: script
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
If-None-Match: "3b9aca00-10d"
If-Modified-Since: Thu, 01 Jan 1970 00:00:01 GMT

HTTP/1.1 304 Not Modified
Cache-Control: no-cache
Etag: "3b9aca00-10d"
Last-Modified: Thu, 01 Jan 1970 00:00:01 GMT
Date: Tue, 22 Aug 2023 18:23:41 GMT
Server: Python/3.10 aiohttp/3.8.5
Access-Control-Expose-Headers: Etag,Server,Date
Access-Control-Allow-Origin: https://steamloopback.host
Access-Control-Allow-Credentials: true

GET /frontend/chunk-385b4644.js HTTP/1.1
Host: localhost:1337
Connection: keep-alive
Accept: */*
Origin: https://steamloopback.host
User-Agent: Mozilla/5.0 (X11; Linux x86_64; Valve Steam Client/default/1692390949) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
Sec-Fetch-Site: cross-site
Sec-Fetch-Mode: cors
Sec-Fetch-Dest: script
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
If-None-Match: "3b9aca00-27ce"
If-Modified-Since: Thu, 01 Jan 1970 00:00:01 GMT

HTTP/1.1 304 Not Modified
Cache-Control: no-cache
Etag: "3b9aca00-27ce"
Last-Modified: Thu, 01 Jan 1970 00:00:01 GMT
Date: Tue, 22 Aug 2023 18:23:41 GMT
Server: Python/3.10 aiohttp/3.8.5
Access-Control-Expose-Headers: Etag,Server,Date
Access-Control-Allow-Origin: https://steamloopback.host
Access-Control-Allow-Credentials: true

GET /frontend/chunk-2b92aa30.js HTTP/1.1
Host: localhost:1337
Connection: keep-alive
Accept: */*
Origin: https://steamloopback.host
User-Agent: Mozilla/5.0 (X11; Linux x86_64; Valve Steam Client/default/1692390949) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
Sec-Fetch-Site: cross-site
Sec-Fetch-Mode: cors
Sec-Fetch-Dest: script
Referer: http://localhost:1337/frontend/chunk-385b4644.js
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
If-None-Match: "3b9aca00-6bc1"
If-Modified-Since: Thu, 01 Jan 1970 00:00:01 GMT

HTTP/1.1 304 Not Modified
Cache-Control: no-cache
Etag: "3b9aca00-6bc1"
Last-Modified: Thu, 01 Jan 1970 00:00:01 GMT
Date: Tue, 22 Aug 2023 18:23:42 GMT
Server: Python/3.10 aiohttp/3.8.5
Access-Control-Expose-Headers: Etag,Server,Date
Access-Control-Allow-Origin: https://steamloopback.host
Access-Control-Allow-Credentials: true

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

Now that I look at it, I'm pretty sure the Cache-Control header can just be removed, because it's re-requesting everything anyway.

@AAGaming00
Copy link
Member

Maybe we can rework it to be in an external file that's minified at runtime? (or even at build time - I'm not sure if there's any decent tool for JS minification you can easily drive from Python).

but why though? this adds extra complexity for a simple bit of js still easily readable that will not change often. if you need to audit it and really can't read that, prettify it externally. we don't need to add more stuff for the loader to handle at init.

trying to be clever and handle If-Modified-Since
interesting, we can probably just null out whatever header that corresponds to, id rather do that than add a cachebust key. can you show an http exchange where it doesn't 304 so I can compare?

encoding the version into the binary is something I've always wanted to do, but idk how to do it sanely

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

but why though? this adds extra complexity for a simple bit of js still easily readable that will not change often. if you need to audit it and really can't read that, prettify it externally. we don't need to add more stuff for the loader to handle at init.

I guess it's fine if it's only ever used as a preloader of sorts. I just don't like code that's hard to diff, but this may be too much work.

interesting, we can probably just null out whatever header that corresponds to, id rather do that than add a cachebust key. can you show an http exchange where it doesn't 304 so I can compare?

It's pretty deep in aiohttp: https://github.com/aio-libs/aiohttp/blob/36bc68768bcef4ab0d95d0e8ba0047565ebc6e09/aiohttp/web_fileresponse.py#L162

We'd have to patch out a big chunk of the static file handling to avoid it, or intercept and modify the request, neither of which sounds good to me.

encoding the version into the binary is something I've always wanted to do, but idk how to do it sanely

There's a few ways, I can draft up a PR if you're interested.

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

(also, just as a throwaway idea for now, would you be open to replacing aiohttp with something less, uh, spaghetti flavored?)

@AAGaming00
Copy link
Member

I just don't like code that's hard to diff, but this may be too much work.

yeah that's fair, but it's just really a bootstrapper for the main decky script and likely won't often (if ever) be changed.

replacing aiohttp

maybe eventually.

once the websocket stuff is done (which uses aiohttp but can be ported more easily to something else) we can consider it

version encoding

yeah id be interested in a PR for that, go ahead

K900 added 2 commits August 22, 2023 23:14
Temporary (hopefully) workaround for NixOS.
A better fix will require more invasive refactoring.
This avoids the If-Modified-Since logic in aiohttp and makes
sure the script is always reloaded.
@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

Force-pushed a version with minified JS.

@TrainDoctor TrainDoctor requested a review from AAGaming00 August 22, 2023 22:20
@TrainDoctor TrainDoctor added the enhancement New feature or request label Aug 22, 2023
@K900
Copy link
Contributor Author

K900 commented Aug 25, 2023

@AAGaming00 so what do you think of the current state of things? I'll have time to look into better version metadata this weekend probably, but this change will already help us a lot.

@TrainDoctor TrainDoctor requested a review from a team August 25, 2023 16:07
@K900 K900 mentioned this pull request Aug 25, 2023
4 tasks
@TrainDoctor TrainDoctor requested review from AAGaming00 and removed request for AAGaming00 August 29, 2023 06:21
@K900
Copy link
Contributor Author

K900 commented Sep 2, 2023

Ping? We're vendoring the patches for now, so there's no real rush, but it would be nice to get this into a release.

@SkyLeite
Copy link
Contributor

SkyLeite commented Sep 9, 2023

I haven't tested it (because there isn't really much to test) but this seems fine to me. Having to make changes to accommodate NixOS is annoying and I would usually suggest doing this kind of patching on NixOS's side (as I've unfortunately done many times), but this seems like a fairly inconsequential change

@K900
Copy link
Contributor Author

K900 commented Sep 9, 2023

I'd say the cache invalidation thing should definitely be in mainline, because it's not just NixOS that will zero out timestamps for reproducibility, other distros do it too, so will hit this issue too. The DECKY_VERSION thing is fundamentally a hack that can be removed with #531.

@K900
Copy link
Contributor Author

K900 commented Nov 10, 2023

Not going to bother rebasing this separately, see #531.

@K900 K900 closed this Nov 10, 2023
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.

4 participants