SteamDeckHomebrew / decky-loader

A plugin loader for the Steam Deck.
https://decky.xyz
GNU General Public License v2.0
4.26k stars 154 forks source link

replace platform chmod with os.chmod #541

Closed Jan200101 closed 7 months ago

Jan200101 commented 9 months ago

Please tick as appropriate:

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

Replaces platform specific chmod implementation with os.chmod os.chmod is already compatible with Linux and Windows and is better at achieving the intended behavior on Windows than a noop.

Also gets rid of potential complicates with unique chmod implementations

suchmememanyskill commented 9 months ago

Other than that, looks fine to me. Works as expected on windows after changing os.chown to os.chmod

suchmememanyskill commented 9 months ago

image shutil.rmtree or windows' del command will fail if the files are read-only. I'm unsure how to solve this in the current PR, as setting permissions in windows is very messy

Jan200101 commented 9 months ago

I don't think that should be a deal breaker considering support for Windows is unofficial at best.

This happens because of a semantic difference in what "read only" means on both systems, the closest to it on Linux would be "immutable".

jurassicplayer commented 9 months ago

While Windows support is unofficial, making a change that will leave artifacts behind (that the common user might not know how to remove) when it was previously workable seems inconsiderate to Windows users. I honestly don't like that decky-loader even unofficially supports Windows, but actively making their user experience worse sounds unappealing.

Perhaps some additional considerations can be made that temporarily revert the permissions to normal so that plugins can be cleaned up/updated/etc. and then reapplied after performing the operation. It doesn't sound entirely ideal either since it would still probably be messy, but it's an idea.

Jan200101 commented 9 months ago

Talk about common users is irrelevant, to install Decky on Windows you already need experience since there are no installers or scripts to do it for you.

For plugin removal we can simply make it writable again (as in, 0o0777)

suchmememanyskill commented 9 months ago

I don't think that should be a deal breaker considering support for Windows is unofficial at best.

This happens because of a semantic difference in what "read only" means on both systems, the closest to it on Linux would be "immutable".

So uh, what's the point of this PR? To break stuff? I thought the point was to use something that has better interop between win and linux.

There's a reason why i stubbed out windows originally, as its permissions are simply a mess.

PartyWumpus commented 9 months ago

You could put the new implementation in the linux file, but then there's really not much point afaict, unless it's faster (or if it was more readable/maintainable, but imo it's about the same). As it is I see no point in breaking windows compatibility (which some of the devs use) for seemingly no benefit?

Please explain the benefits further if you can, thanks.

Edit: also, currently chmod is completely disabled (on purpose!) if decky is run as the user, because it assumes it's being managed by something else (nixos for example), which your implementation removes.

Jan200101 commented 9 months ago

I thought the point was to use something that has better interop between win and linux.

The point was to use a pure python implementation instead of shelling out.

There's a reason why i stubbed out windows originally, as its permissions are simply a mess.

Its not a mess, its simply different to how unix did it. Which is why code needs to be written to expect every scenario, in this case the fact you may not be able to delete unwritable files.

the simplest solution would be to readd the euid check the sanest solution would be to make plugins writable again before deleting

Please explain the benefits further if you can, thanks.

also, currently chmod is completely disabled (on purpose!) if decky is run as the user, because it assumes it's being managed by something else (nixos for example), which your implementation removes.

So the whole making plugins read-only to prevent tempering isn't even available everywhere?

What else would be managing plugin permissions? I don't see how NixOS would do it considering plugins may be dynamically installed.

_get_effective_user_id is Linux only, so a Window stub would need to be added for it

suchmememanyskill commented 9 months ago

So the whole making plugins read-only to prevent tempering isn't even available everywhere?

What else would be managing plugin permissions? I don't see how NixOS would do it considering plugins may be dynamically installed.

_get_effective_user_id is Linux only, so a Window stub would need to be added for it

Decky on Windows currently always runs as an unpriviledged user. Windows is a lot more flexible in what it can do with a user account, so i don't see the need to run Decky as admin.

Good point for bringing this up btw. If the read-only flag is just there for tamper protection from users, wouldn't this be a bit pointless on Windows? Explorer completely ignores it.

Jan200101 commented 9 months ago

If the read-only flag is just there for tamper protection from users, wouldn't this be a bit pointless on Windows

I guess so, though I do recall hearing that it helps with plugins potentially messing with one another, I am unsure how likely such a scenario is.

There are multiple ways I can see this continue:

suchmememanyskill commented 9 months ago

I personally am in favor of the former option. If needs be we can re-evaluate this when decky for windows actually gets used.

If anyone else on the team thinks the latter option is better, please leave a comment.

jurassicplayer commented 9 months ago

I also agree with the former option. There is already an organizational split and at the moment, I think any solutions we do come up with would keep the same structure or litter the equivalent of if (IsWindows): do something around.

PartyWumpus commented 7 months ago

Works well, thanks (sorry this one took so long to test as well)