Skip to content

rust-installer/install-template.sh: improve efficiency, step 1. #145809

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+44 −53
Open

rust-installer/install-template.sh: improve efficiency, step 1. #145809

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29e205f
rust-installer/install-template.sh: improve efficiency, step 1.
he32 Aug 24, 2025
27a0720
Avoid an extra case/esac: use a flag instead.
he32 Oct 18, 2025
9248435
Evidently tidy doesn't like tabs for indentation, even in shell scrip…
he32 Oct 18, 2025
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
97 changes: 44 additions & 53 deletions src/tools/rust-installer/install-template.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,54 +551,47 @@ 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

# HACK: Try to support overriding --docdir. Paths with the form
# "share/doc/$product/" can be redirected to a single --docdir
# path. If the following detects that --docdir has been specified
# then it will replace everything preceding the "$product" path
# component. The problem here is that the combined rust installer
# contains two "products": rust and cargo; so the contents of those
# directories will both be dumped into the same directory; and the
# contents of those directories are _not_ disjoint. Since this feature
# is almost entirely to support 'make install' anyway I don't expect
# 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
fi
local _is_bin=false
case "$_file" in
etc/*)
local _f="$(echo "$_file" | sed 's/^etc\///')"
_file_install_path="$CFG_SYSCONFDIR/$_f"
Comment on lines +557 to +558
Copy link
Member

@hkBst hkBst Oct 19, 2025

Choose a reason for hiding this comment

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

Since this is Bash, we also don't need to use sed to cut off the front of this string, but can use ${_file#*/} to cut off the first part up to and including the first slash.

Copy link
Contributor Author

@he32 he32 Oct 19, 2025

Choose a reason for hiding this comment

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

Yes, I know. This isn't just because this is Bash, it is because it's a POSIX Bourne Shell (which also has that construct). I have a slew of similar changes lined up as "step 2" of the efficiency improvements for this script. So once this pull request gets accepted & applied (I hope that will happen...), a new pull request will follow. Ref. my comments about performance testing above. But since my earlier attempt at getting this all accepted stalled or was ignored, I decided it would be tactically better to try to divide this up in bite-size changes.

Copy link
Member

@hkBst hkBst Oct 20, 2025

Choose a reason for hiding this comment

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

I appreciate that, and I see that your (much) earlier attempt did include such changes. I'm not sure the solution is dribbling things out though, especially when related changes get brought up in review. It feels like a waste of my review efforts. Perhaps you could reopen a separate PR with all the changes you had lined up before regarding speeding up this particular script with all my comments to this one addressed? I feel I'm more likely to approve such a PR (not that I have any formal power to do that, but it may count for something...). Given that lack of bash expertise is one of the reasons given for the slow review I'd also recommend commenting all these constructs.

;;
bin/*)
local _f="$(echo "$_file" | sed 's/^bin\///')"
_is_bin=true
_file_install_path="$CFG_BINDIR/$_f"
;;
lib/*)
local _f="$(echo "$_file" | sed 's/^lib\///')"
_file_install_path="$CFG_LIBDIR/$_f"
;;
share/man/*)
local _f="$(echo "$_file" | sed 's/^share\/man\///')"
_file_install_path="$CFG_MANDIR/$_f"
;;
share/doc/*)
# HACK: Try to support overriding --docdir. Paths with the form
# "share/doc/$product/" can be redirected to a single --docdir
# path. If the following detects that --docdir has been specified
# then it will replace everything preceding the "$product" path
# component. The problem here is that the combined rust installer
# contains two "products": rust and cargo; so the contents of those
# directories will both be dumped into the same directory; and the
# contents of those directories are _not_ disjoint. Since this feature
# is almost entirely to support 'make install' anyway I don't expect
# this problem to be a big deal in practice.
if [ "$CFG_DOCDIR" != "<default>" ]
then
local _f="$(echo "$_file" | sed 's/^share\/doc\/[^/]*\///')"
_file_install_path="$CFG_DOCDIR/$_f"
fi
;;
share/*)
local _f="$(echo "$_file" | sed 's/^share\///')"
_file_install_path="$CFG_DATADIR/$_f"
;;
Comment on lines +569 to +593
Copy link
Member

@hkBst hkBst Oct 19, 2025

Choose a reason for hiding this comment

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

This could use a nested case statement: after matching "share/" remove it via ${_file#*/}, then match on the remainder. (This would also prevent repeated matching of "share/".)

Copy link
Contributor Author

@he32 he32 Oct 19, 2025

Choose a reason for hiding this comment

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

There is no danger of "multiple matches". The man page for sh(1) on NetBSD says:

     The syntax of the case command is

           case word in
           [(] pattern) [list] ;&
           [(] pattern) [list] ;;
           ...
           esac

     The pattern can actually be one or more patterns (see Shell Patterns
     described later), separated by "|" characters.

     word is expanded and matched against each pattern in turn, from first to
     last, with each pattern being expanded just before the match is
     attempted.  When a match is found, pattern comparisons cease, and the
     associated list, if given, is evaluated.  If the list is terminated with
     ";&" execution then falls through to the following list, if any, without
     evaluating its pattern, or attempting a match.  When a list terminated
     with ";;" has been executed, or when esac is reached, execution of the
     case statement is complete.  The exit status is that of the last command
     executed from the last list evaluated, if any, or zero otherwise.

and I am pretty certain that this is identical to what POSIX specifies. So there should be no need to further complicate this construct.

Copy link
Member

@hkBst hkBst Oct 20, 2025

Choose a reason for hiding this comment

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

I'm not talking about danger; this is just another optimization.

Copy link
Contributor Author

@he32 he32 Oct 28, 2025

Choose a reason for hiding this comment

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

It is ... unproven that this makes much of a difference performance-wise. Can you benchmark what difference it would make?

Copy link
Member

@hkBst hkBst Oct 28, 2025

Choose a reason for hiding this comment

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

If for some reason, you have a preference for this code to remain written in a way in which it quite obviously does more work than necessary, and have benchmarks that show that it matters only negligibly, then I'll grudgingly accept that, but it still wouldn't be a very good reason not to write it the efficient way. The efficient way is also not any more complex, so I can't see any downsides. Can you?

esac

# Make sure there's a directory for it
make_dir_recursive "$(dirname "$_file_install_path")"
Expand All @@ -617,13 +610,11 @@ install_components() {

maybe_backup_path "$_file_install_path"

if echo "$_file" | grep "^bin/" > /dev/null || test -x "$_src_dir/$_component/$_file"
then
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 755 "$_file_install_path"
if $_is_bin || test -x "$_src_dir/$_component/$_file"; then
Copy link
Member

@hkBst hkBst Oct 19, 2025

Choose a reason for hiding this comment

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

If we're testing whether files are executable to decide the mod bits to set, then why do we also need to check whether they are in the bin/ directory? Are there perhaps some files in the bin/ directory that are missing the executable bit, and we should instead add those missing bits?

Copy link
Contributor Author

@he32 he32 Oct 19, 2025

Choose a reason for hiding this comment

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

I am just trying to faithfully 1:1 replicate the effect of what the original code does, and it has this test. If this should be changed, I would posit that is a separate change.

Copy link
Member

@hkBst hkBst Oct 20, 2025

Choose a reason for hiding this comment

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

I don't know if it should be changed, but since I'm noticing this as part of my review I thought I'd bring it up as something that doesn't seem right.

Copy link
Contributor Author

@he32 he32 Oct 28, 2025

Choose a reason for hiding this comment

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

I cannot vouch for all the install components thrown at this script. I am merely trying to faithfully reproduce the actions the script already did before this suggested change.

run chmod 755 "$_file_install_path"
else
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 644 "$_file_install_path"
run chmod 644 "$_file_install_path"
fi
critical_need_ok "file creation failed"

Expand Down
Loading