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

Remove sudo requirement from install & uninstall scripts #3056

Merged
merged 1 commit into from Jul 21, 2021

Conversation

luispadron
Copy link
Collaborator

@luispadron luispadron commented Jun 4, 2021

Resolves #2806
Request for comments document (if applies):

Short description πŸ“

This PR removes the need to require sudo access. This is problematic in contexts where TTY is not available to enter password (such as in CI/CD). This also makes it more clear that the install script isn't doing anything crazy on someones machine that would require sudo.

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@luispadron luispadron force-pushed the luis/remove-sudo-from-install branch from eb79832 to 7ceee72 Compare June 4, 2021 20:58
@luispadron luispadron changed the title Remove sudo requirement from install script Remove sudo requirement from install & uninstall scripts Jun 4, 2021
@fortmarek
Copy link
Member

Do we know why sudo was there in the first place?

@luispadron
Copy link
Collaborator Author

@fortmarek Im not entirely sure! @pepibumur would you have more context?

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

I'm OK with this being merged but I would very much curious why it was there in the first place cc @pepibumur

@pepicrft
Copy link
Contributor

@fortmarek Im not entirely sure! @pepibumur would you have more context?

@luispadron I remember having issues when running the installation scripts in M1. It'd be great if the scripts could be tested in that environment. If they work I think it's ok to merge this PR.

@RLovelett
Copy link

RLovelett commented Jun 23, 2021

This is a bit of a blocker for some to even use Tuist. For instance, in our environment sudo is not something we have access to on our managed computers. I worked around this by running the steps of the install script manually.

Also, perhaps adopting a $HOME/.local is also a good fallback if /usr/bin/local is not writable by the current user. I'm less sure I think this is a good general solution. It works for me but I'm not sure it works for everyone.

@fortmarek
Copy link
Member

This is a bit of a blocker for some to even use Tuist. For instance, in our environment sudo is not something we have access to on our managed computers. I worked around this by running the steps of the install script manually.

This is unfortunate! I agree it'd be much better if we got around using sudo.

Also, perhaps adopting a $HOME/.local is also a good fallback if /usr/bin/local is not writable by the current user. I'm less sure I think this is a good general solution. It works for me but I'm not sure it works for everyone.

yeah, I'm not sure how often /usr/bin/local is not writable by the user? User could also run the whole install command with sudo to get around that, afaict. We could add some error handling checking the access rights for that directory and then showing error with helpful information if it's actually not writable.

Before merging this, we need someone to test this on M1. @luispadron is currently on vacation, but I believe he should return back soon-ish, so we can move forward with this afterwards.

@pepicrft
Copy link
Contributor

Before merging this, we need someone to test this on M1. @luispadron is currently on vacation, but I believe he should return back soon-ish, so we can move forward with this afterwards.

I can test this one on M1 and merge it if everything works as expected.

@pepicrft pepicrft force-pushed the luis/remove-sudo-from-install branch from 7ceee72 to fd073d7 Compare July 21, 2021 08:19
@pepicrft
Copy link
Contributor

I confirm this works successfully on M1 so I'll go ahead and merge the PR.

@pepicrft pepicrft merged commit 2f6282d into main Jul 21, 2021
@pepicrft pepicrft deleted the luis/remove-sudo-from-install branch July 21, 2021 08:19
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.

Feature: Add check for sudo in install script.
4 participants