install.sh: Simplify syntax and improvements #9223

supechicken · 2024-01-26T20:45:29Z

Changes

Check for sudo availability in a more elegant way
Group constants in a place
Import /etc/lsb-release directly instead of using grep
Replace all $(id -u) calls with ${EUID}
Align comments
Put repository URLs into variables
Remove pixz since .pixz should be backward compatible with xz
Only extract files specified in git sparse-checkout from the repo tarball
Simplify syntax
Use array on BOOTSTRAP_PACKAGES instead of string

Not tested yet

satmandu · 2024-01-26T20:53:28Z

Please try in containers!

Zopolis4 · 2024-01-26T21:45:12Z

install.sh

+: "${GTIHUB_OWNER:=chromebrew}"
+: "${GITHUB_REPO:=chromebrew}"
+: "${GITHUB_BRANCH:=master}"
+: "${GITLAB_REPO:=26210301}"


To be honest, I don't know if there's any practical value to changing the gitlab repo, I don't know of any other repos that mirror our package repository or would be otherwise useful to switch to.

This is used to set the PREBUILT_URL variable below to reduce duplicate long assignments. This is actually good because if this value changes in the future, we only have to change in one place...here.

Wouldn't that be the same if we just had the value set in PREBUILT_URL? I'm not a fan of single-use variables.

Zopolis4 · 2024-01-26T21:46:47Z

install.sh

@@ -102,15 +109,17 @@ fi

 if [[ "$ARCH" == "x86_64" ]]; then
  LIB_SUFFIX='64'
+else
+  LIB_SUFFIX=''


Is LIB_SUFFIX not already empty by default, since we don't set the variable?

It doesn't hurt to set it explicitly for clarity and in the off chance it's set outside of crew.

Reduce?

LIB_SUFFIX='' [[ "${ARCH}" == "x86_64" ]] && LIB_SUFFIX='64'

Zopolis4 · 2024-01-26T21:51:48Z

install.sh

@@ -194,16 +200,16 @@ for package in $BOOTSTRAP_PACKAGES; do
  # This is really ugly, FIXME after #7082 is merged.


Obviously this isn't something you have to do, but if you wanted you could change the code to not have two copies of each url based on the tar compression, and instead use the value of binary_compression now that that's a possibility.

If not, I'll get around to it soon.

uberhacker · 2024-01-27T00:14:48Z

install.sh


 # Check if the script is being run as root.
 if [ "${EUID}" == "0" ]; then
  echo_error "Chromebrew should not be installed or run as root."
-  echo_info "Please login as 'chronos' and restart the install."
+  echo_info  "Please login as 'chronos' and restart the install."


Extra space?

uberhacker · 2024-01-27T00:14:55Z

install.sh

  exit 1
 fi

 # Reject crostini.
 if [[ -d /opt/google/cros-containers && "${CREW_FORCE_INSTALL}" != '1' ]]; then
  echo_error "Crostini containers are not supported by Chromebrew :/"
-  echo_info "Run 'CREW_FORCE_INSTALL=1 exec bash --init-file <(curl -Ls git.io/vddgY)' to perform install anyway"
+  echo_info  "Run 'CREW_FORCE_INSTALL=1 exec bash --init-file <(curl -Ls git.io/vddgY)' to perform install anyway"


Extra space?

uberhacker · 2024-01-27T00:17:59Z

install.sh

  echo_error "Please run the installer in the VT-2 shell."
-  echo_info "To start the VT-2 session, type Ctrl + Alt + ->"
+  echo_info  "To start the VT-2 session, press Ctrl + Alt + -> and login with user 'chronos'"


Extra space?

End sentence with a .?

uberhacker · 2024-01-27T00:19:09Z

install.sh

-    echo_info "This will delete ALL files in ${CREW_PREFIX}!"
-    echo_info "Continue?"
+    echo_info  "This will delete ALL files in ${CREW_PREFIX}!"
+    echo_info  "Continue?"


Extra space?

uberhacker · 2024-01-27T00:19:44Z

install.sh

-  export "$(grep CHROMEOS_RELEASE_CHROME_MILESTONE /etc/lsb-release)"
-else
-  echo_info "Unable to detect system information, installation will continue."
+if ! [[ "${CHROMEOS_RELEASE_TRACK}" == 'stable-channel' || "${CREW_FORCE_INSTALL}" == '1' ]]; then


if [[ -z "${CREW_FORCE_INSTALL}" && "${CHROMEOS_RELEASE_TRACK}" != 'stable-channel' ]]; then?

uberhacker · 2024-01-27T00:41:04Z

install.sh

+: "${CREW_CACHE_DIR:=$CREW_PREFIX/tmp/packages}"
+
+BOOTSTRAP_PACKAGES=(zstd crew_mvdir ruby git ca_certificates openssl)
+SPARSE_CHECKOUT=(packages manifest/${USER_SPACE_ARCH} lib bin crew tests tools)


USER_SPACE_ARCH not defined yet?

uberhacker · 2024-01-27T00:46:29Z

install.sh

-  echo_info "If this happens, please report them to: https://github.com/chromebrew/chromebrew/issues"
-  echo_info "If the install fails, try running 'CREW_AMD_INSTALL=1 exec bash --init-file <(curl -Ls git.io/vddgY)'"
+  echo_info "If this happens, please report them to: ${REPO_URL}/issues"
+  echo_info "If the install fails, try running 'CREW_AMD_INSTALL=1 exec bash --init-file <(curl -L git.io/vddgY)'"


Now you are starting to think like me. I don't like when ANY command is silenced.

uberhacker · 2024-01-27T01:04:09Z

install.sh

    else
-      urls+=("https://gitlab.com/api/v4/projects/26210301/packages/generic/${name}/${version}_${ARCH}/${name}-${version}-chromeos-${ARCH}.tar.zst")
+      urls+=("${PREBUILT_URL}/${name}/${version}_${ARCH}/${name}-${version}-chromeos-${ARCH}.tar.zst")


Reduce "${PREBUILT_URL}/${name}/${version}_${ARCH}/${name}-${version}-chromeos-${USER_SPACE_ARCH}.tar.${ARCHIVE_EXTENSION}" to one line and simply assign ARCHIVE_EXTENSION instead?

And we don't need to assign ARCHIVE_EXTENSION, we can just use the binary_compression value in each package.

(once this is rebased onto master, this review is no longer relevant)

uberhacker · 2024-02-03T08:03:33Z

install.sh

@@ -3,56 +3,81 @@
 # Exit on fail.
 set -e

-RESET='\e[0m'
+# Default chromebrew repo values.
+: "${GTIHUB_OWNER:=chromebrew}"


GITHUB_OWNER?

uberhacker · 2024-03-11T17:08:45Z

@supechicken: Did you want to merge master into this PR branch and resolve merge conflicts?

install.sh: Simplify syntax

d55d14c

supechicken requested review from uberhacker, satmandu and Zopolis4 January 26, 2024 20:45

supechicken added 2 commits January 27, 2024 04:48

Align messages

9e8a390

Make shellcheck happy with array

008900f

Make shellcheck happy with array

49a3c9e

Zopolis4 requested changes Jan 26, 2024

View reviewed changes

uberhacker reviewed Jan 27, 2024

View reviewed changes

Zopolis4 mentioned this pull request Feb 1, 2024

Rework installer, change USER_SPACE_ARCH #9241

Merged

uberhacker reviewed Feb 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install.sh: Simplify syntax and improvements #9223

install.sh: Simplify syntax and improvements #9223

supechicken commented Jan 26, 2024 •

edited

satmandu commented Jan 26, 2024

Zopolis4 Jan 26, 2024

uberhacker Jan 27, 2024

Zopolis4 Jan 28, 2024

Zopolis4 Jan 26, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024

Zopolis4 Jan 26, 2024

Zopolis4 Jan 26, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024 •

edited

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024

uberhacker Jan 27, 2024 •

edited

Zopolis4 Jan 28, 2024

Zopolis4 Feb 11, 2024

uberhacker Feb 3, 2024

uberhacker commented Mar 11, 2024

		@@ -194,16 +200,16 @@ for package in $BOOTSTRAP_PACKAGES; do
		# This is really ugly, FIXME after #7082 is merged.

install.sh: Simplify syntax and improvements #9223

Are you sure you want to change the base?

install.sh: Simplify syntax and improvements #9223

Conversation

supechicken commented Jan 26, 2024 • edited

Changes

Not tested yet

satmandu commented Jan 26, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uberhacker Jan 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uberhacker Jan 27, 2024 • edited

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

uberhacker commented Mar 11, 2024

supechicken commented Jan 26, 2024 •

edited

uberhacker Jan 27, 2024 •

edited

uberhacker Jan 27, 2024 •

edited