Bash installer #256

itchyny · 2020-01-23T15:20:31Z

Hi, I looked into the installer and translated into bash one line by one. Needs some more works (~~not sure how to compare version string.. maybe I'll implement this with arrays~~ parameter substitution works). I love Homebrew, thank you all.

Why?

resolves Port installer to Bash #217
Future versions of macOS won’t include scripting language runtimes by default, and might require you to install additional packages. If your software depends on scripting languages, it’s recommended that you bundle the runtime within the app. (49764202) https://developer.apple.com/documentation/macos_release_notes/macos_catalina_10_15_release_notes

but shell script is hard to write

I introduced shellcheck integration. This tool catches various common mistakes in shell script and there's no reason to follow it.

a few notes

Intentional changes
- Basically I ported keeping the behavior of the original script but I intentionally introduced some changes.
  - Since user_chmods are already chmod 755, there's no need to chmod zsh_dirs bc13488
  - Print a newline after the donation link 589c653
  - Print audible bell only when the stdout is tty 4f0094f
Always check the length of the array when iterating through it
- for x in ${arr[@]} splits an element containing a space, for x in "${a[@]}" aborts with unbound variable when set -u, "${a[@]-}" emits an empty string when the array is empty, "${a[@]+"${a[@]}"}" (see https://stackoverflow.com/questions/7577052) works actually but the syntax is ugly. Checking the length is much simpler and readable.
Including Linuxbrew to this script is not that difficult (maybe). Note that the flags of stat is not portable.

All changes addressed.

MikeMcQuaid · 2020-02-27T20:23:06Z

@itchyny Did a full review of this, some testing and made some tweaks/fixes where I saw them. As a starting point: this is an incredible first contribution, I'm really impressed 👏. I think it's now at a stage where I'm happy to have this merged. I just want to have a few other maintainers give their 👀 on it and ensure I only merge this at a time where I can be around to make any quick fixes should problems pop up. Let me know what you think of any of my changes. Again: great work!

@Homebrew/maintainers would appreciate as many of you as possible taking a look at this and offering any thoughts. Thanks!

vitorgalvao · 2020-02-27T20:49:47Z

install.sh

+
+# Use an extra newline and bold to avoid this being missed.
+ohai "Homebrew has enabled anonymous aggregate formulae and cask analytics."
+echo "$(cat <<EOS


Why echo and cat at once? cat alone should suffice.

vitorgalvao · 2020-02-27T20:54:15Z

install.sh

+# This script installs to /usr/local only. To install elsewhere (which is
+# unsupported) you can untar https://github.com/Homebrew/brew/tarball/master
+# anywhere you like.
+HOMEBREW_PREFIX="/usr/local"


Bash has a lot of pitfalls and places where things can go wrong if we’re distracted.

I’d feel safer if every variable would be readonly when possible (i.e. if it’s not, you know it will change) and used single quotes unless double quotes are necessary.

vitorgalvao · 2020-02-27T20:55:13Z

install.sh

+tty_reset="$(tty_escape 0)"
+
+shell_join() {
+  local arg


Similarly, local variables should be local -r (local and readonly).

vitorgalvao · 2020-02-27T20:55:48Z

install.sh

+MACOS_OLDEST_SUPPORTED="10.13"
+
+# no analytics during installation
+export HOMEBREW_NO_ANALYTICS_THIS_RUN=1


To make a variable readonly and exported at the same time, declare -rx works.

vitorgalvao · 2020-02-27T20:57:34Z

install.sh

+
+####################################################################### script
+if [[ "$OSTYPE" == "linux-gnu" ]]; then
+  abort "$(cat <<'EOABORT'


Again, it’s weird to me that a function is called and then cat is called inside. In this case, there’s also the case that EOABORT is quoted, which is inconsistent with the rest of the code.

vitorgalvao · 2020-02-27T20:59:26Z

install.sh

+}
+
+should_install_command_line_tools() {
+  if version_gt "$macos_version" "10.13"; then


All variables should be ${var} instead of $var. That’s one of those simple things that can save us from a world of trouble by doing everywhere, and we definitely do not want this script to fail due to mistakes.

In addition, it makes the script consistent throughout, since several cases already use the former notation.

vitorgalvao · 2020-02-27T21:04:52Z

install.sh

+  ohai "You are using macOS ${macos_version}."
+  ohai "${who} do not provide support for this ${what}."
+
+  echo "$(cat <<EOS


Again, I don’t get the use of cat, as echo alone would suffice. And we can make it look nicer by keeping the indentation and removing it on output, like so:

echo " Some message With ${variables} " | sed -E 's/^ {2}//'

Replacing the 2 with however many spaces we have at the start of the line.

I personally prefer simpler and less magical (no undenting).

cat <<EOS Hello, world! EOS

I disagree that the sed call is either complex or magical, especially when you consider the script as a whole, but I won’t bikeshed it. Not using echo and cat at the same time should be considered, though.

There’s also

cat <<-EOS Hello World EOS

The <<- removes the indentation (not as good as Ruby’s <<~), but it only works with tabs.

Undenting is also probably not worth the complexity, agreed.

Or we could just echo every line separately, which also aligns nicely with ohai, as seen below.

dawidd6 · 2020-02-28T00:14:18Z

Initial Linux support on top of this PR branch: https://github.com/dawidd6/install/tree/install-sh-linux.

vitorgalvao · 2020-02-28T14:19:27Z

install.sh

+if [[ "$OSTYPE" == "linux-gnu" ]]; then
+  abort "$(cat <<'EOABORT'
+  To install Homebrew on Linux, paste at a terminal prompt:
+    sh -c "$(curl -fsSL https://raw.githubusercontent.com/Linuxbrew/install/master/install.sh)"


We should be explicit (/bin/bash -c). /bin/sh is often a link to something else and can change (e.g. in Ubuntu it used to point to bash and later dash), which might mean unexpected breakage when we least expect it.

But it's a sh script actually, not bash.

https://github.com/Linuxbrew/install/blob/master/install.sh

Look at the shebang.

@dawidd6 in this PR it's #!/bin/bash. On most systems /bin/sh runs bash or some other interpreter in compatibility mode, IIRC.

When you run sh -c "…" the shebang line is ignored. The script will be run using sh, not with whatever the shebang line specifies. We should either remove Bash-isms from the script and test with dash, or change the installation instructions to bash -c "…".

MikeMcQuaid · 2020-03-02T12:36:53Z

@vitorgalvao and others: I appreciate the style suggestions but as this PR is otherwise ready to go (and I'm limited in the windows in which I can merge it and it's limiting other things going in until it's done) I'm going to merge as-is and accept PRs to tweak style. Ideally all style checks will be enforced by shellcheck or similar.

The sh -c I'm also leaving as-is as it's what was in the existing code (which I think is worthing sticking as close to as possible for this PR) and it should go away as soon as we get Linux support in here.

MikeMcQuaid · 2020-03-02T12:37:20Z

Thanks so much for your first, incredible contribution! Without people like you submitting PRs we couldn't run this project. You rock, @itchyny!

itchyny · 2020-03-02T13:02:06Z

Great, thank you all for throughout reviews! I'm honored and I love Homebrew!

See also: - https://itchyny.hatenablog.com/entry/2020/03/03/100000 - Homebrew/install#256 - Homebrew/install#217

itchyny added 30 commits January 23, 2020 20:42

git mv install install.sh

1a209f5

change the shebang to bash

8f3e824

setup variables in bash

d4882e0

implement string formatters in bash

689acee

implement ohai and warn in bash

16b5bc0

implement execute function in bash

6f1c11e

implement execute_sudo in bash

704d1a1

implement getc and wait_for_user in bash

b2dd833

set -u in bash

5ddf034

comment out utilities to implement later

c59f27f

set $USER using id -un in bash

9f6c22d

invalidate credentials in bash

dd4f8ff

change directory to /usr in bash

895a645

implement OS check in bash

30b8edc

print the install targets in bash

5c7bbd4

define group_chmods, zsh_dirs and mkdirs in bash

0f218f7

implement user_only_chmod and user_chmods in bash

dfe4171

implement chmods, chowns and chgrps in bash

e8cab39

echo the permission and owner changes in bash

733e3ea

check command line tools and wait for user input in bash

67cb8f0

fix for empty array

3754a73

change owner and permissions in bash

7d1f4a8

install Command Line Tools for XCode in bash

493954a

implement Xcode license check in bash

d4a6892

implement the rest of the script in bash

a6ae341

restore the Ruby installer

0351602

switch to Bash installer in CI

b9a4418

fix newlines

890d0ea

[nit] fix chomp style

2bd0f72

implement version comparison in bash

c9abfc6

MikeMcQuaid added 5 commits February 27, 2020 20:05

Merge remote-tracking branch 'origin/master' into install-sh

76b35de

install: tweak Ruby installer deprecation message.

e690a46

install.sh: tweak formatting, messages and comments.

f61ae9a

install.sh: exit rather than return from top level.

af460c8

install.sh: add missing execute wrapper calls.

a31d675

MikeMcQuaid mentioned this pull request Feb 27, 2020

index: use Bash installer. Homebrew/brew.sh#463

Merged

MikeMcQuaid mentioned this pull request Feb 27, 2020

Port installer to Bash #217

Closed

MikeMcQuaid self-assigned this Feb 27, 2020

MikeMcQuaid added the do not merge label Feb 27, 2020

MikeMcQuaid approved these changes Feb 27, 2020

View reviewed changes

vitorgalvao reviewed Feb 27, 2020

View reviewed changes

vitorgalvao reviewed Feb 28, 2020

View reviewed changes

MikeMcQuaid merged commit f782bb3 into Homebrew:master Mar 2, 2020

hidehiro98 mentioned this pull request Mar 5, 2020

Update homebrew install lewagon/setup#122

Merged

machupicchubeta added a commit to machupicchubeta/laptop that referenced this pull request Mar 15, 2020

In favor of "Homebrew" install script migration by "Bash"

4a6d960

See also: - https://itchyny.hatenablog.com/entry/2020/03/03/100000 - Homebrew/install#256 - Homebrew/install#217

machupicchubeta mentioned this pull request Mar 15, 2020

In favor of "Homebrew" install script migration by "Bash" machupicchubeta/laptop#72

Merged

lock bot added the outdated label Apr 2, 2020

lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bash installer #256

Bash installer #256

itchyny commented Jan 23, 2020 •

edited

Loading

MikeMcQuaid commented Feb 27, 2020

vitorgalvao Feb 27, 2020

vitorgalvao Feb 27, 2020 •

edited

Loading

vitorgalvao Feb 27, 2020

vitorgalvao Feb 27, 2020

vitorgalvao Feb 27, 2020

vitorgalvao Feb 27, 2020

vitorgalvao Feb 27, 2020 •

edited

Loading

sjackman Feb 27, 2020

vitorgalvao Feb 27, 2020

MikeMcQuaid Feb 28, 2020

reitermarkus Mar 1, 2020

dawidd6 commented Feb 28, 2020

vitorgalvao Feb 28, 2020 •

edited

Loading

dawidd6 Feb 28, 2020

zbeekman Feb 28, 2020

sjackman Feb 28, 2020

MikeMcQuaid commented Mar 2, 2020

MikeMcQuaid commented Mar 2, 2020

itchyny commented Mar 2, 2020

Bash installer #256

Bash installer #256

Conversation

itchyny commented Jan 23, 2020 • edited Loading

Why?

but shell script is hard to write

a few notes

MikeMcQuaid commented Feb 27, 2020

Choose a reason for hiding this comment

vitorgalvao Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitorgalvao Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidd6 commented Feb 28, 2020

vitorgalvao Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 2, 2020

MikeMcQuaid commented Mar 2, 2020

itchyny commented Mar 2, 2020

itchyny commented Jan 23, 2020 •

edited

Loading

vitorgalvao Feb 27, 2020 •

edited

Loading

vitorgalvao Feb 27, 2020 •

edited

Loading

vitorgalvao Feb 28, 2020 •

edited

Loading