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

fix(forge): pre-emptively create lib dir if it doesn't exist for updating submodules #6521

Merged
merged 8 commits into from
Dec 4, 2023

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Dec 4, 2023

Motivation

Closes #6519

Solution

Check if there are any submodules at all in the repo, and only update if so.

@Evalir Evalir requested a review from mattsse December 4, 2023 18:17
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

the failing test look related

the issue here is that we want to update a folder that does not exist.

I think we should instead only perform the update if the folder already exists.

@Evalir
Copy link
Member Author

Evalir commented Dec 4, 2023

Yeah so what's happening here is an edge case for prb-proxy: The reason why we wanna be able to run submodule update on an empty lib file is that we could have missing deps that might have been removed (manually), and we need to reinstall them through a .gitmodules. prob-proxy doesn't have one, so it will just not install any deps at all.

I think the fix here is to check if the .gitmodules file exists, and if not, skip the submodule update. If it exists, even if the dir is empty we wanna run the update as we could have missing deps.

@Evalir Evalir requested a review from mattsse December 4, 2023 18:50
crates/forge/bin/cmd/install.rs Outdated Show resolved Hide resolved
@Evalir Evalir requested a review from mattsse December 4, 2023 19:24
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

can we add a sanity test for this:

empty project + call forge install

@mattsse mattsse merged commit 13af418 into master Dec 4, 2023
19 checks passed
@mattsse mattsse deleted the evalir/fix-pathing-lib branch December 4, 2023 23:13
@PaulRBerg
Copy link
Contributor

Thanks for the quick turnaround 🙏

@PaulRBerg
Copy link
Contributor

There's still a problem left, though .. running forge install will add the following setting in foundry.toml:

libs = ["node_modules", "lib"]

Would it be possible to have Forge not do this?

@sambacha
Copy link
Contributor

sambacha commented Dec 5, 2023

Yeah so what's happening here is an edge case for prb-proxy: The reason why we wanna be able to run submodule update on an empty lib file is that we could have missing deps that might have been removed (manually), and we need to reinstall them through a .gitmodules. prob-proxy doesn't have one, so it will just not install any deps at all.

I think the fix here is to check if the .gitmodules file exists, and if not, skip the submodule update. If it exists, even if the dir is empty we wanna run the update as we could have missing deps.

please do not do this, some people may use git submodules for non-foundry reasons.

This issue also closes my issue #2649

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

Successfully merging this pull request may close these issues.

Running "forge install" in a project with no submodule dependencies does not work
5 participants