-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow non-admin user installs on macOS if prefix is user-writable #948
base: master
Are you sure you want to change the base?
Conversation
Allow macOS installation without sudo if prefix is user-writable Previously, the installer always aborted if sudo was unavailable on macOS. This change adds a check to determine if the Homebrew prefix is already writable by the current user. If writable, the script proceeds without sudo. Otherwise, it aborts with a clear message instructing users how to create and adjust permissions on the Homebrew prefix, enabling standard (non-admin) users to install Homebrew in that directory. Developed and tested on 15.3.1
Used brew style --fix install.sh
@@ -247,7 +252,18 @@ have_sudo_access() { | |||
|
|||
if [[ -n "${HOMEBREW_ON_MACOS-}" ]] && [[ "${HAVE_SUDO_ACCESS}" -ne 0 ]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if [[ -n "${HOMEBREW_ON_MACOS-}" ]] && [[ "${HAVE_SUDO_ACCESS}" -ne 0 ]] | |
if [[ -n "${HOMEBREW_ON_MACOS-}" ]] && [[ "${HAVE_SUDO_ACCESS}" -ne 0 && ! -w "${HOMEBREW_PREFIX}" ]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that if it's not writable then that's handled by the earlier part, but this extra check would be okay if you think it is good to have.
install.sh
Outdated
if [[ -n "${HOMEBREW_ON_MACOS-}" ]] && [[ -w "${HOMEBREW_PREFIX}" ]] | ||
then | ||
return 1 | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should put this in the have_sudo_access()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to go for total minimal changes. Everything seems to work smoothly in the script if:
- the homebrew prefix exists and is writable and
have_sudo_access
returns false and- the script doesn't
abort
My goal was minimally invasive but handles the above.
Other issues like permissions and the CLT are handled in other parts of the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't do a 'return 1' early, or something similar, then the script will prompt for sudo when it really doesn't actually need it to function and end up with a "This user is not in the sudoers file".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better solution is to add a new function:
have_sudo_access_or_prefix_write_permission() {
return [[ -n "${HOMEBREW_ON_MACOS-}" && -w "${HOMEBREW_PREFIX}" ]] || have_sudo_access
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that's good
What if the directories under it are not writable? |
Thanks for the PR!
This is my question, too. Can CLT install also be done without Stepping back: can you explain the motivation for this? |
I debated trying to do some kind of 'make sure it's empty' but then I did some testing and the script itself actually does a lot of |
You are welcome! Motivation: CLT can be installed without sudo, strangely enough. I ran "python3" in a terminal, got a GUI prompt that said it needed developer tools and had a blue install button and clicking the blue install button worked fine to install the CLT tools. |
I think having a "make sure it's empty" or other checks on permissions here is necessary to merge this, sorry.
This won't install it using the installer, though, which we want to preserve if not using See Lines 837 to 856 in 839ae50
Every other use of |
Would it be worth having this with a check of && ! The I did want the change to be minimally invasive so if auditing execute sudo lines is required a separate function maybe is better? |
I guess |
This is a larger change but it: 1. Has a check to make sure all files in the Homebrew prefix are writable 2. Specifically handles CLT installation 3. Cleanly separates it out into its own function Uses running `chmod` as a check. If it fails then something isn't writable. Bonus is it also is a permissions repair. For the CLT install with macOS sudoless mode, the first method will fail, so within the fallback we check for macOS_sudoless_mode and if we're using it then `abort` with the message about manual installation, instead of running the GUI pop up manual installation. This still retains the original goal of installing in the official prefixes without sudo, so it doesn't result in an "Alternative (unsupported) installation".
This is a larger change but it:
Uses running For the CLT install with macOS sudoless mode, the first method will fail, so within the fallback we check for macOS_sudoless_mode and if we're using it then This still retains the original goal of installing in the official prefixes without sudo, so it doesn't result in an "Alternative (unsupported) installation". Every use of
|
Line Number | Command Usage (via execute_sudo) | Check in macos_sudoless_check() handles checking write access | Already world-writable by default | Likely needs sudo |
---|---|---|---|---|
800 | execute_sudo "${CHMOD[@]}" "u+rwx" "${chmods[@]}" | ✅ | ||
804 | execute_sudo "${CHMOD[@]}" "g+rwx" "${group_chmods[@]}" | ✅ | ||
808 | execute_sudo "${CHMOD[@]}" "go-w" "${user_chmods[@]}" | ✅ | ||
812 | execute_sudo "${CHOWN[@]}" "${USER}" "${chowns[@]}" | ✅ | ||
816 | execute_sudo "${CHGRP[@]}" "${GROUP}" "${chgrps[@]}" | ✅ | ||
819 | execute_sudo "${INSTALL[@]}" "${HOMEBREW_PREFIX}" | ✅ | ||
824 | execute_sudo "${MKDIR[@]}" "${mkdirs[@]}" | ✅ | ||
825 | execute_sudo "${CHMOD[@]}" "ug=rwx" "${mkdirs[@]}" | ✅ | ||
828 | execute_sudo "${CHMOD[@]}" "go-w" "${mkdirs_user_only[@]}" | ✅ | ||
830 | execute_sudo "${CHOWN[@]}" "${USER}" "${mkdirs[@]}" | ✅ | ||
831 | execute_sudo "${CHGRP[@]}" "${GROUP}" "${mkdirs[@]}" | ✅ | ||
836 | execute_sudo "${MKDIR[@]}" "${HOMEBREW_REPOSITORY}" | ✅ | ||
838 | execute_sudo "${CHOWN[@]}" "-R" "${USER}:${GROUP}" "${HOMEBREW_REPOSITORY}" | ✅ | ||
844 | execute_sudo "${MKDIR[@]}" "${HOMEBREW_CACHE}" | ✅ | ||
851 | execute_sudo "${CHMOD[@]}" "g+rwx" "${HOMEBREW_CACHE}" | ✅ | ||
855 | execute_sudo "${CHOWN[@]}" "-R" "${USER}" "${HOMEBREW_CACHE}" | ✅ | ||
859 | execute_sudo "${CHGRP[@]}" "-R" "${GROUP}" "${HOMEBREW_CACHE}" | ✅ | ||
871 | execute_sudo "${TOUCH[@]}" "${clt_placeholder}" | ✅ | ||
884 | execute_sudo "/usr/sbin/softwareupdate" "-i" "${clt_label}" | ❌ | ||
885 | execute_sudo "/usr/bin/xcode-select" "--switch" "/Library/Developer/CommandLineTools" | ❌ | ||
887 | execute_sudo "/bin/rm" "-f" "${clt_placeholder}" | ✅ | ||
909 | execute_sudo "/usr/bin/xcode-select" "--switch" "/Library/Developer/CommandLineTools" | ❌ |
return "${MACOS_SUDOLESS_SATISFIED}" | ||
fi | ||
|
||
[[ -d "${HOMEBREW_PREFIX}" ]] && "${CHMOD[@]}" -R "ug+w" "${HOMEBREW_PREFIX}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should test (read) the file mode rather than change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing would be a somewhat convoluted find
and if there is an issue and you can fix it with a chmod
, it does not make sense to do a find
then a chmod
if just the initial chmod
would work and handle both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user does have sudo access and there are many many files under the prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Doing a check then bailing out at the first sign of not writable is a way to make it less bad.
If there are a lot of files, and the user does have sudo
, then yes, as it is now and also with find
it would do a large check over all of them.
There is a chicken and egg of what comes first of running without sudo
and making sure it can run without sudo
.
My thoughts go to a begin/rescue or try/except pattern fitting the issue well but that’s a larger change.
|
||
sudo mkdir -p "${HOMEBREW_PREFIX}" | ||
sudo chown -R "${USER}:${GROUP}" "${HOMEBREW_PREFIX}" | ||
sudo chmod -R "ug+w" "${HOMEBREW_PREFIX}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a good idea to add write permission to the group for all files/dirs. For example, you should remove the write access after installation:
chmod -R go-w "${HOMEBREW_PREFIX}/share/zsh"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An earlier objection was "What if the directories under it are not writable?"
so then if we only want some to be writable then
What is a clean way to determine what should have write and what shouldn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the permission for the user is enough, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just matching other chmod
s in the script. But yes just the user is enough, agreed.
The check, either a direct One thought is to try to craft the most sane |
Replacing this: With this: Would be more efficient if that's acceptable. |
I took a new homebrew install prefix directory and copied it so I had 31 copies, for a total of:
And did chmod -R and compared it with parallel xargs with various args per cmd (-n). Commands Tested in BenchmarkStandard recursive chmodchmod -R u+w /opt/homebrew/padding_area Parallel xargs with optimal configuration (n=32)find "/opt/homebrew/padding_area" -print0 | xargs -0 -P 14 -n 32 chmod u+w The benchmark tested this parallel approach with different values for the Results Summary
So for 250k files it took 5 to 6 seconds. |
I'm increasingly tempted to pass on this, sorry @willmcginnis. The complexity required to make this work does not seem to match the number of people who need this functionality (as far as I can tell: perhaps only @willmcginnis). For this to get merged it needs to:
|
Thank you the reasons Mike. I agree I haven’t seen it discussed but maybe because was assumed to not be possible. One option that you might be okay with that could satisfy those is through the use of a Placing just below
The shortest would be:
The other option I see is a function with more checks and an |
If this seems like less code that the current version: this seems like a good idea to both implement and document in the README 👍🏻 |
Allow macOS installation without sudo if prefix is user-writable
Previously, the installer always aborted if sudo was unavailable on macOS.
This change adds a check to determine if the Homebrew prefix is already writable by the current user.
If writable, the script proceeds without sudo.
Otherwise, it aborts with a clear message instructing users how to create and adjust permissions on the Homebrew prefix, enabling standard (non-admin) users to install Homebrew in that directory.
Developed and tested on 15.3.1