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.

Sign up for GitHub

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

install.sh: Perfomance: Use more shell builtins #106

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

install.sh: Perfomance: Use more shell builtins #106

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d7ed69
install.sh: Perfomance: Use more shell builtins
mbargull Feb 19, 2021
53d557b
install.sh: Cache abs_path and make_dir_recursive
mbargull Feb 22, 2021
2ccfecc
install.sh: Use test -d for make_dir_recursive
mbargull Apr 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 38 additions & 53 deletions install-template.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,11 @@ valopt() {
eval $v="$default"
for arg in $CFG_ARGS
do
if echo "$arg" | grep -q -- "--$op="
then
local val=$(echo "$arg" | cut -f2 -d=)
eval $v=$val
fi
case "${arg}" in
"--${op}="* )
Copy link
Author

Choose a reason for hiding this comment

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

FYI, this is technically a behavioral change because the original code grepped for "--$op" instead of "^--$op" -- testing for the prefix is probably intended, though. Same for the --$option case below.

local val="${arg#--${op}=}"
eval $v=$val
esac
done
putvar $v
else
Expand Down Expand Up @@ -280,10 +280,10 @@ validate_opt () {
done
for option in $VAL_OPTIONS
do
if echo "$arg" | grep -q -- "--$option="
then
is_arg_valid=1
fi
case "${arg}" in
"--${option}="* )
is_arg_valid=1
esac
done
if [ "$arg" = "--help" ]
then
Expand All @@ -302,8 +302,8 @@ validate_opt () {

absolutify() {
local file_path="$1"
local file_path_dirname="$(dirname "$file_path")"
local file_path_basename="$(basename "$file_path")"
local file_path_dirname="${file_path%/*}"
local file_path_basename="${file_path##*/}"
Comment on lines +308 to +309
Copy link
Author

Choose a reason for hiding this comment

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

NB: These dirname/basename substitutions assume more well-formed paths, e.g., no trailing slashes etc.
Let me know if you aren't comfortable with adding these constraints and if I should revert to dirname/basename.

local file_abs_path="$(abs_path "$file_path_dirname")"
local file_path="$file_abs_path/$file_path_basename"
# This is the return value
Expand Down Expand Up @@ -442,8 +442,8 @@ uninstall_components() {
local _directive
while read _directive; do

local _command=`echo $_directive | cut -f1 -d:`
local _file=`echo $_directive | cut -f2 -d:`
local _command="${_directive%%:*}"
local _file="${_directive#*:}"

# Sanity checks
if [ ! -n "$_command" ]; then critical_err "malformed installation directive"; fi
Expand Down Expand Up @@ -549,8 +549,8 @@ install_components() {
local _directive
while read _directive; do

local _command=`echo $_directive | cut -f1 -d:`
local _file=`echo $_directive | cut -f2 -d:`
local _command="${_directive%%:*}"
local _file="${_directive#*:}"

# Sanity checks
if [ ! -n "$_command" ]; then critical_err "malformed installation directive"; fi
Expand All @@ -559,35 +559,23 @@ install_components() {
# Decide the destination of the file
local _file_install_path="$_dest_prefix/$_file"

if echo "$_file" | grep "^etc/" > /dev/null
then
local _f="$(echo "$_file" | sed 's/^etc\///')"
_file_install_path="$CFG_SYSCONFDIR/$_f"
fi

if echo "$_file" | grep "^bin/" > /dev/null
then
local _f="$(echo "$_file" | sed 's/^bin\///')"
_file_install_path="$CFG_BINDIR/$_f"
fi

if echo "$_file" | grep "^lib/" > /dev/null
then
local _f="$(echo "$_file" | sed 's/^lib\///')"
_file_install_path="$CFG_LIBDIR/$_f"
fi

if echo "$_file" | grep "^share" > /dev/null
then
local _f="$(echo "$_file" | sed 's/^share\///')"
_file_install_path="$CFG_DATADIR/$_f"
fi

if echo "$_file" | grep "^share/man/" > /dev/null
then
local _f="$(echo "$_file" | sed 's/^share\/man\///')"
_file_install_path="$CFG_MANDIR/$_f"
fi
case "${_file}" in
etc/* )
_file_install_path="$CFG_SYSCONFDIR/${_file#etc/}"
;;
bin/* )
_file_install_path="$CFG_BINDIR/${_file#bin/}"
;;
lib/* )
_file_install_path="$CFG_LIBDIR/${_file#lib/}"
;;
share/man/* )
_file_install_path="$CFG_MANDIR/${_file#share/man/}"
;;
share/* )
_file_install_path="$CFG_DATADIR/${_file#share/}"
;;
esac

# HACK: Try to support overriding --docdir. Paths with the form
# "share/doc/$product/" can be redirected to a single --docdir
Expand All @@ -601,15 +589,14 @@ install_components() {
# this problem to be a big deal in practice.
if [ "$CFG_DOCDIR" != "<default>" ]
then
if echo "$_file" | grep "^share/doc/" > /dev/null
then
local _f="$(echo "$_file" | sed 's/^share\/doc\/[^/]*\///')"
_file_install_path="$CFG_DOCDIR/$_f"
fi
case "${_file}" in
share/doc/* )
_file_install_path="$CFG_DOCDIR/${_file#share/doc/*/}"
esac
fi

# Make sure there's a directory for it
make_dir_recursive "$(dirname "$_file_install_path")"
make_dir_recursive "${_file_install_path%/*}"
critical_need_ok "directory creation failed"

# Make the path absolute so we can uninstall it later without
Expand All @@ -625,7 +612,7 @@ install_components() {

maybe_backup_path "$_file_install_path"

if echo "$_file" | grep "^bin/" > /dev/null || test -x "$_src_dir/$_component/$_file"
if test -z "${_file##bin/*}" || test -x "$_src_dir/$_component/$_file"
Copy link

Choose a reason for hiding this comment

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

test -z "${_file##bin/*}" unintentionally accepts the empty string; [ "${_file#bin/}" != "$_file" ] would be a slightly more faithful translation. (Presumably this doesn’t really matter.)

Or there’s [[ "$_file" = bin/* ]] since this is #!/bin/bash, although the script seems to otherwise avoid bashisms so I guess that avoidance should be preserved.

Copy link
Author

Choose a reason for hiding this comment

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

${_file} is never empty here because of https://github.com/rust-lang/rust-installer/pull/106/files#diff-ec68db39ae4ea5bfe559a47a8a880d5d383dfa81ba067652d2adfcc3c0cd2a17R571 .
So,

  1. if test -z "${_file##bin/*}" || ...
  2. if [ "${_file#bin/*}" != "${_file}" ] || ...
  3. if case "${_file}" in bin/*) ;; *) false ; esac || ... (POSIX-y [[ "${_file}" = bin/*)
    should all be equivalent here.
    If someone has a strong preference for any of those options (I don't), then I'm happy to change it accordingly!

then
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 755 "$_file_install_path"
Expand Down Expand Up @@ -770,8 +757,6 @@ verbose_msg

need_cmd mkdir
need_cmd printf
need_cmd cut
need_cmd grep
need_cmd uname
need_cmd tr
need_cmd sed
Expand Down