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 1 commit into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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"
;;
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"
;;
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

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"
bin/*)
run cp "$_src_dir/$_component/$_file" "$_file_install_path"
run chmod 755 "$_file_install_path"
;;
Comment on lines +616 to +619
Copy link
Member

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

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

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...
*)
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