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

Merged
merged 3 commits into from
Nov 17, 2025
+44 −53
Merged

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

Changes from 1 commit
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 52 additions & 54 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,45 @@ 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
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\///')"
_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 +608,20 @@ 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 test -x "$_src_dir/$_component/$_file"; then
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 755 "$_file_install_path"
else
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 644 "$_file_install_path"
case "$_file" in
Copy link
Contributor

@Shunpoco Shunpoco Oct 6, 2025

Choose a reason for hiding this comment

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

I feel that hiring case statement only for one pattern is too much. How about using a flag?

 local _is_bin=0 case "$_file" in etc/*) local _f="$(echo "$_file" | sed 's/^etc\///')" _file_install_path="$CFG_SYSCONFDIR/$_f" ;; bin/*) local _f="$(echo "$_file" | sed 's/^bin\///')" _file_install_path="$CFG_BINDIR/$_f" _is_bin=1 # Turn the flag on ;; # .... esac # ...  if [ $_is_bin -eq 1 ] || test -x "$_src_dir/$_component/$_file"; then # use the flag here run cp "$_src_dir/$_component/$_file" "$_file_install_path" run chmod 755 "$_file_install_path" else run cp "$_src_dir/$_component/$_file" "$_file_install_path" run chmod 644 "$_file_install_path"
Copy link
Contributor Author

@he32 he32 Oct 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Something like that should be doable. Stylistically I would instead of 0 and 1 use "true" and "false", since I can then drop the "test" via [. The file-copying could also be moved outside of the test. However...

case / esac are shell built-ins, and are therefore comparatively cheap, as they do not require a new process be fork()ed. Using grep for the pattern matching, as the original code did is what's expensive...

Let's see if I can draft something along the lines suggested.
...now done.
bin/*)
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 755 "$_file_install_path"
;;
Copy link
Member

@hkBst hkBst Aug 24, 2025

Choose a reason for hiding this comment

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

This seems like a useful change of behavior, but you did not mention it in your PR summary.
Copy link
Contributor Author

@he32 he32 Aug 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am a little uncertain what "this" refers to. There was no intention to change the behavior of the script. Note that the test in the previous "if" had an "or", so both tests needs to be covered going forward, and that is what I think the new code does. The case construct can only cover the test for the path name, and cannot test for the executeable-ness of the file.
Copy link
Member

@hkBst hkBst Aug 24, 2025

Choose a reason for hiding this comment

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

Ah right, I misread. Still I don't understand why these couldn't be combined like in the original code, perhaps by first testing for "bin"ness and saving the result in a variable, then doing the if-or?
Copy link
Member

@hkBst hkBst Oct 6, 2025

Choose a reason for hiding this comment

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

The original code contains 2 copies of "run cp ...; run chmod ...", your code contains 3 copies, but ideally we'd have a single copy...
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.

The two latest commits should have tidied up this issue, there is now just one "run cp ..." command in the pointed-to spot.
*)
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 644 "$_file_install_path"
;;
esac
fi
critical_need_ok "file creation failed"

Expand Down
Loading