Skip to content

feat(forge) run script install dependencies #9885

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

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

Conversation

programskillforverification
Copy link
Contributor

Motivation

close #9837

Solution

Move Installer to forge-verify, so both forge and forge-script could use it. Keep install argument on original file and forge install still work.

@zerosnacks
Copy link
Member

Hi @programskillforverification thanks for your PR!

Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

@jenpaff
Copy link
Collaborator

jenpaff commented Mar 25, 2025

Hi @programskillforverification thanks for your PR!

Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

hey @programskillforverification ! will you be able to push this PR over the finish line? alternatively we can take it over

@jenpaff jenpaff moved this to Ready For Review in Foundry Mar 25, 2025
@jenpaff jenpaff moved this from Ready For Review to In Progress in Foundry Mar 25, 2025
@jenpaff jenpaff added this to the v1.1.0 milestone Mar 25, 2025
@programskillforverification
Copy link
Contributor Author

Hi @programskillforverification thanks for your PR!
Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

hey @programskillforverification ! will you be able to push this PR over the finish line? alternatively we can take it over

I am sorry that I forget this pr. I will finish it asap.

@programskillforverification
Copy link
Contributor Author

Hi @programskillforverification thanks for your PR!

Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

Install command also belongs to verify. According to our community, it's better that "have forge verify-contract do forge install too". Or split this pr into two, one is for verify, the other is for script.

@programskillforverification programskillforverification force-pushed the forge-script-install-dependencies branch from 6d5c1e3 to 6cc97a7 Compare March 26, 2025 06:54
@grandizzy
Copy link
Collaborator

Hi @programskillforverification thanks for your PR!
Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

Install command also belongs to verify. According to our community, it's better that "have forge verify-contract do forge install too". Or split this pr into two, one is for verify, the other is for script.

hey @programskillforverification what @zerosnacks suggests is to externalize code from crates/verify/src/install.rs into it's own crate / commons and reuse in both install and verify, does this make sense? @zerosnacks pls chime in in case I I got your comment wrong. thanks

@jenpaff jenpaff modified the milestones: v1.1.0, v1.2.0 Apr 15, 2025
@jenpaff jenpaff modified the milestones: v1.2.0, v1.3.0 May 5, 2025
@jenpaff
Copy link
Collaborator

jenpaff commented May 5, 2025

Hi @programskillforverification thanks for your PR!
Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

Install command also belongs to verify. According to our community, it's better that "have forge verify-contract do forge install too". Or split this pr into two, one is for verify, the other is for script.

hey @programskillforverification what @zerosnacks suggests is to externalize code from crates/verify/src/install.rs into it's own crate / commons and reuse in both install and verify, does this make sense? @zerosnacks pls chime in in case I I got your comment wrong. thanks

hey @programskillforverification ! Bump on the previous message, would you be able to take this PR over the finish line, otherwise happy to take over / assign to someone else. just let us know

@programskillforverification
Copy link
Contributor Author

Hi @programskillforverification thanks for your PR!
Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

Install command also belongs to verify. According to our community, it's better that "have forge verify-contract do forge install too". Or split this pr into two, one is for verify, the other is for script.

hey @programskillforverification what @zerosnacks suggests is to externalize code from crates/verify/src/install.rs into it's own crate / commons and reuse in both install and verify, does this make sense? @zerosnacks pls chime in in case I I got your comment wrong. thanks

hey @programskillforverification ! Bump on the previous message, would you be able to take this PR over the finish line, otherwise happy to take over / assign to someone else. just let us know

I will finish right now

@programskillforverification
Copy link
Contributor Author

Hi @programskillforverification thanks for your PR!
Instead of moving the install command into the verify folder where it arguably does not belong it may make more sense to split it out into its own individual crate.

Install command also belongs to verify. According to our community, it's better that "have forge verify-contract do forge install too". Or split this pr into two, one is for verify, the other is for script.

hey @programskillforverification what @zerosnacks suggests is to externalize code from crates/verify/src/install.rs into it's own crate / commons and reuse in both install and verify, does this make sense? @zerosnacks pls chime in in case I I got your comment wrong. thanks

Hey, @grandizzy, do you means move to crates/common? I once try but can't solve cyclic package dependency

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

Successfully merging this pull request may close these issues.

forge script should install dependencies
5 participants