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

@eregon eregon commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

@eregon

eregon commented Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

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

havenwood
havenwood requested changes May 22, 2026
View reviewed changes

@havenwood havenwood left a comment

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 seems like a nice fix for #522. I left a couple suggestions around propagating errors up and clearing the temp dir, but LGTM!

Comment thread share/ruby-install/util.sh Outdated
Comment thread share/ruby-install/util.sh Outdated
Comment thread share/ruby-install/util.sh Outdated
@eregon
eregon force-pushed the reliable-extraction branch from 809dfcf to c6c9b7a Compare May 27, 2026 20:29
@eregon
eregon requested a review from havenwood May 27, 2026 20:30
@eregon

eregon commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

Would love if this could get merged soon :)

havenwood

This comment was marked as outdated.

Sign in to view

havenwood
havenwood requested changes May 27, 2026
View reviewed changes
Comment thread share/ruby-install/util.sh Outdated
Comment thread share/ruby-install/util.sh Outdated
@havenwood

havenwood commented May 27, 2026

Copy link
Copy Markdown
Collaborator

Would love if this could get merged soon :)

After staring at the diff for a while I got worried about parallel installs and left a few more comments, but everything else looks good to me.

Sorry for not catching that on first review. Getting the tmp dir part right is just the part that gives me pause to merge while postmodern is busy.

@eregon
Fix archive extraction to ensure it always extracts to the given path
7954b02 
* 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
eregon force-pushed the reliable-extraction branch from c6c9b7a to 7954b02 Compare May 27, 2026 21:58
@eregon

eregon commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you for the thorough review, all fixed now and I also re-tested extensively locally, all good.

@eregon
eregon requested a review from havenwood May 27, 2026 21:59
@eregon

eregon commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Could this important fix be merged?

@havenwood

havenwood commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Could this important fix be merged?

Yes, I think we should merge. Postmodern is busy and I'd really like to test it thoroughly before release and haven't had a chance. Will do!

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

Reviewers

@havenwood havenwood Awaiting requested review from havenwood
1 more reviewer
@omarharby805-ship-it omarharby805-ship-it omarharby805-ship-it approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug truffleruby+graalvm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ruby-install truffleruby-graalvm-33.0.0 is broken

3 participants

@eregon @havenwood @omarharby805-ship-it