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

replace platform chmod with os.chmod #541

Merged
merged 1 commit into from
Nov 11, 2023

Conversation

Jan200101
Copy link
Contributor

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

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
Copy link
Contributor

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

@suchmememanyskill
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

PartyWumpus commented Sep 15, 2023

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
Copy link
Contributor Author

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.

  • simplified execution
    • the previous implementation needed to start a new process which naively received the function arguments as parameters without any kind of checks
  • verifiable
    • the previous implementation simply used whatever chmod was on the path. We can't know what that implementation actually does.
  • availability
    • While its rare that chmod is not available on a system, it is indeed a possible scenario. os.chmod is available everywhere you can run python

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
Copy link
Contributor

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.

@TrainDoctor TrainDoctor reopened this Sep 15, 2023
@Jan200101
Copy link
Contributor Author

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:

  • readd the windows stub and replace the linux version with the new code
  • make every call to chmod on windows enable writing (e.g. turning 555 into 777)
    • This would still allow for files to be made unreadable or unexecutable

@suchmememanyskill
Copy link
Contributor

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
Copy link
Contributor

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 PartyWumpus merged commit 7c3ae9b into SteamDeckHomebrew:main Nov 11, 2023
6 checks passed
@PartyWumpus
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants