Skip to content
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.

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

feat(foundryup): add foundryup self-update #9609

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

9547
Copy link
Contributor

@9547 9547 commented Dec 31, 2024

Motivation

Add a --self-update command to update foundryup itself.

Solution

foundryup/foundryup Outdated Show resolved Hide resolved
foundryup/foundryup Outdated Show resolved Hide resolved
foundryup/foundryup Outdated Show resolved Hide resolved
@9547 9547 requested a review from zerosnacks January 2, 2025 13:41
@zerosnacks
Copy link
Member

zerosnacks commented Jan 3, 2025

Thanks @9547!

cc @grandizzy what do you think of adding this into the installer?

It would make upgrading foundryup easier for end users and the functionality is in line with rustup.

One concern is that where the rest of the Bash script is highly flexible in allowing users to install from forks we would limit foundryup to be updated only from the source repo. This makes sense from a security standpoint.

Another concern is that it is possible a failure case (e.g. partial download) could possibly brick foundryup. One way to address this is to avoid writing directly to the final location, and instead first write to a temporary location, perform a sanity check and then move it to the final location.

cc @sambacha do you have opinions or concerns regarding this proposed update?

@grandizzy
Copy link
Collaborator

One concern is that where the rest of the Bash script is highly flexible in allowing users to install from forks we would limit foundryup to be updated only from the source repo. This makes sense from a security standpoint.

Yep, agree is better to update only from the source repo.

Another concern is that it is possible a failure case (e.g. partial download) could possibly brick foundryup. One way to address this is to avoid writing directly to the final location, and instead first write to a temporary location, perform a sanity check and then move it to the final location.

That would be nice to have, though probably unlikely to happen given foundryup size (and could happen also in the current way to update foundryup, by rerunning curl -L https://foundry.paradigm.xyz | bash), I don't have a strong opinion if we should add or not.

@DaniPopes
Copy link
Member

Downloading to a temp file first is required with bash script because the interpreter (bash) is not guaranteed to buffer the entire file in memory when executing. This means that modifying the file while it's running it could cause inconsistencies and weird errors.

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

per comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants