Misc. installer tweaks and fixes #15068

Merged
merged 4 commits into from Oct 2, 2012

Conversation

Projects
None yet
3 participants
Contributor

Sharpie commented Sep 23, 2012

Addresses #14996 but also includes a couple of usability tweaks.

Member

mxcl commented Sep 24, 2012

Please, no to: 56276b4

The installer only installs to /usr/local. It will never install anywhere else. I hate abstracting such things into constants.

Member

mxcl commented Sep 24, 2012

I still think we should just exit and tell the user what to do. The installer is for installing, we don't accept any further tasks. This is an edge-case. What should be done in edge-cases is the user's choice. It is outside the scope of Homebrew's installer to chmod /usr/local if it's for some reason got permissions that prevent us even looking inside it.

Member

mxcl commented Sep 24, 2012

Rest is great. Good improvements and well-caught on d60493b.

Contributor

Sharpie commented Sep 24, 2012

I hate abstracting such things into constants.

Yeah, but I really, really hate grepping through a file making multiple changes in order to redirect the installer to /usr/foo so I can debug things like #14996. On top of that, trying to craft a commit that contains the actual bugfix but not the multiple redirections to /usr/foo is an exercise in frustration.

Changing one line at the top to put the installer in debug mode and then using git add -p is much easier.

Member

mxcl commented Sep 24, 2012

Fair points. I retire my complaint.

Contributor

Sharpie commented Sep 24, 2012

It is outside the scope of Homebrew's installer to chmod /usr/local if it's for some reason got permissions that prevent us even looking inside it.

I can pull that commit. However, since we do take steps to chmod everything inside /usr/local, it seems in-scope to fix permissions on the top level directory as well (every other subdirectory in /usr is mode 755 on a fresh install).

If we're going to draw a line on what the installer does, it seems like we should either:

  1. Ensure /usr/local is setup correctly to host a Homebrew installation.
  2. Abort if /usr/local already exists at all and tell the user to clean it up.

I don't know if implementing option 1 half-way by fixing the subdirectories, but not fixing the top level directory is the best way to go.

Member

mxcl commented Sep 24, 2012

The scope of the installer is to install to /usr/local. I'd argue that if we can't even get at /usr/local, then changing its permissions is outside of the scope.

It's an edge-case, if this was something that happens a lot, I'd change my mind.

My policy on edge case stuff is: don't do anything, abort with helpful text. Any code you write may have side-effects and more bugs. Pushing the work onto the user is reasonable, they have more smarts than our script and may even know why the directory is non-accessible (maybe they put it like that).

Sharpie added some commits Sep 23, 2012

@Sharpie Sharpie Abstract installation into a variable
The installation prefix is now stored in `HOMEBRW_PREFIX` which makes it easier
to adapt the installation script or to temporarily re-direct the installation
for testing purposes.
94dbaf0
@Sharpie Sharpie Abort if the HOMEBREW_PREFIX is not searchable
Without executable permissions on the prefix directory, permissions tests such
as `File.writable?` will fail and `File.chdir` will die with a strange error.

Fixes #14996.
96905eb
@Sharpie Sharpie Check all file modes, not just writable
We run a `chmod g+rwx` to set read, write and execute permissions but only use
`File.writable?` to detect which directories need this. Since we are setting
read and execute permissions, we should also use `File.readable?` and
`File.executable?`.
5608ad6
@Sharpie Sharpie Notify user of how to abort installation
Notify users that hitting any key other than ENTER will abort the installer.
0019042
Contributor

Sharpie commented Sep 24, 2012

Allright, edited to abort instead of running chmod.

Contributor

samueljohn commented Oct 2, 2012

This looks great!

Sharpie merged commit 0019042 into Homebrew:go Oct 2, 2012

Contributor

Sharpie commented Oct 2, 2012

Pushed.

xu-cheng locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.