search

XcodesOrg / xcodes

The best command-line tool to install and switch between multiple versions of Xcode.
MIT License
3.6k stars 120 forks source link

Unify `sudo` behavior for installing Xcode and runtimes #252

Closed StevenSorial closed 1 year ago

StevenSorial commented 1 year ago

This PR removes the old sudo implementation, and uses the same behavior used by runtimes .

StevenSorial commented 1 year ago

@MattKiazyk I hope this PR can be merged soon đŸ€žđŸŒ

StevenSorial commented 1 year ago

@MattKiazyk Is this pr a no-go? I was hoping it could be merged, so follow it with other PRs.

MattKiazyk commented 1 year ago

Hi @StevenSorial Thanks for your patience and the time you spent on the original PR.

Some discussions happened about what we want to accomplish with sudo vs requesting the password when we need it and we've come to the conclusion that we don't want to higher permissions if we don't need them for all the functions.

This accomplishes a few things. It doesn't allow some accidental code that needs higher permission from actually getting it and It's being a nice player in open source.

I believe all the higher level privileges that are needed are after download/extract, so it might be a good addition to have a --skip-post-install or something similar where it doesn't approve license, run first time to install additional components, etc.

Thanks for your patience again.

StevenSorial commented 1 year ago

Thank you for your response and sorry for my late reply. I will present some arguments why I think that might not be the right decision but feel free to skip them if it's not open for discussion.

Consistency with runtimes

230 was accepted with this sudo implementation, so this PR just make it consistent across the whole app. It was not a security/stability concern for the runtimes command, so I don’t see why it will be for the other commands.

Not a real concern

DECISIONS.md

  • Run xcodes with sudo: xcodes doesn’t need elevated privileges the entire time, and means simple programmer mistakes can have bad consequences

    It doesn’t allow some accidental code that needs higher permission from actually getting it and It’s being a nice player in open source.

While this concern is legitimate, I think it’s exaggerated. Many of the command line tools we use sometimes require sudo, and we don’t think much about it because they most probably have a good reason why they needed it, and because the community already tried and trusts the developers.

Also, In this PR and in #230, sudo is not always required, and only requested when the user entered a command that will require it, e.g, for runtimes, sudo is not required when only downloading an image or when installing >=iOS 16 image.

Personally, as a user, I would be more concerned if a program is handling my password manually instead of the shell/system

Hacky (IMO)

It’s being a nice player in open source

Maybe I don’t look into many CLI tools code, but I personally have never seen this solution. Moreover, I see it as a maintainability burden. I also had some problems with it while implementing #230 where it sometimes asked for the password while I already had privilege + It doesn’t work when using touchid for sudo.

@MattKiazyk @rogerluan What do you think? I hope that I'm not arguing too much đŸ˜