Skip to content

Fix archive extraction to ensure it always extracts to the given path#524

Open
eregon wants to merge 1 commit into
postmodern:masterfrom
eregon:reliable-extraction
Open

Fix archive extraction to ensure it always extracts to the given path#524
eregon wants to merge 1 commit into
postmodern:masterfrom
eregon:reliable-extraction

Conversation

@eregon
Copy link
Copy Markdown
Contributor

@eregon eregon commented Mar 26, 2026

* Fixes postmodern#522
* The various functions.sh no longer need to hardcode $ruby_dir_name.
* Remove $ruby_dir_name entirely as it is now unused.
@eregon
Copy link
Copy Markdown
Contributor Author

eregon commented Apr 12, 2026

@postmodern Could you review this PR? It fixes a confusing bug: #522

Copy link
Copy Markdown
Collaborator

@havenwood havenwood left a comment

Choose a reason for hiding this comment

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

This seems like a nice fix for #522. I left a couple suggestions around propagating errors up and clearing the temp dir, but LGTM!

local tmp="$dest_dir/tmp"

mkdir -p "$dest" || return $?
rm -rf "$dest"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should probably have a trailing || return $? in case the rm fails. (Just to match project convention, and if it carries on after this rm fails the mv below might confusingly move into $dest instead of renaming.)

error "Multiple extracted directories under $tmp"
return 1
fi
mv "${extracted[0]}" "$dest"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It might be a good idea to add an rm -rf "$tmp" here to clean up the now-empty $tmp dir. (If an rm is added, the mv would need a || return $?.)

local archive="$1"
local dest="${2:-${archive%/*}}"
local dest="$2"
local dest_dir="$(dirname "$2")"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dirname "$dest" might be nice? "$2" and "$dest" are same here, so just an optional nit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ruby-install truffleruby-graalvm-33.0.0 is broken

3 participants