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(dependencies): support pinning of tags / revs when using .gitmodules with foundry.lock #9522

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

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Dec 9, 2024

Motivation

Closes #7225

  • git submodule doesn't have support for pinning to tags/revisions, only branches in .gitmodules
  • This creates unintended behavior in forge when updating deps.
  • Reproduction of unintended behaviour
  1. Install oz with tag forge install OpenZeppelin/[email protected].
  2. git submodule status shows the oz dep checked out at OpenZeppelin/openzeppelin-contracts@69c8def
  3. Run forge update
  4. oz dep is now checked out at the most recent commit of the master branch instead of the commit of the tag.

Solution

Introduce a basic lockfile foundry.lock that consists of a mapping from relative lib_path to DepIdentifier. This file is committed with every forge install.

Every time forge update is run, this file is inferred to correctly check out the dep and maintain tag or rev pinning if specified.

In case we want to override a dep and set a new tag, this can be done like so: forge update owner/dep@new-tag

TODO

  • account for foundry.lock generation the first time.
  • migration tests
  • monorepo test
  • forge remove

@yash-atreya yash-atreya self-assigned this Dec 9, 2024
@yash-atreya yash-atreya marked this pull request as ready for review December 10, 2024 13:22
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

I think this makes sense overall, 2 scenarios was thinking of

  1. update deps of dependencies
  • forge init and then install specific tag forge install OpenZeppelin/openzeppelin-contracts@tag=v4.9.4. This will result in 2 deps in lib/openzeppelin-contracts/lib:
erc4626-tests
forge-std
  • forge update updates dependencies of Oz dependency, resulting in 3 deps
erc4626-tests
forge-std
halmos-cheatcodes

This probably cannot be fixed even when openzeppelin-contracts contracts will add its own forge-submodule-info.json?

  1. updating dependency to a different version
  • cd in lib/openzeppelin-contracts/ and git checkout v5.0.2
  • forge update silently checks out v4.9.4
  • in order to persist then forge-submodule-info.json should be manually edited and "lib/openzeppelin-contracts":{"Tag":"v4.9.4"} updated to "lib/openzeppelin-contracts":{"Tag":"v5.0.2"}

Maybe on forge update we should print out versions (and should follow up with docs / book update).

@yash-atreya
Copy link
Member Author

yash-atreya commented Dec 11, 2024

I think this makes sense overall, 2 scenarios was thinking of

  1. update deps of dependencies
  • forge init and then install specific tag forge install OpenZeppelin/openzeppelin-contracts@tag=v4.9.4. This will result in 2 deps in lib/openzeppelin-contracts/lib:
erc4626-tests
forge-std
  • forge update updates dependencies of Oz dependency, resulting in 3 deps
erc4626-tests
forge-std
halmos-cheatcodes

This probably cannot be fixed even when openzeppelin-contracts contracts will add its own forge-submodule-info.json?

  1. updating dependency to a different version
  • cd in lib/openzeppelin-contracts/ and git checkout v5.0.2
  • forge update silently checks out v4.9.4
  • in order to persist then forge-submodule-info.json should be manually edited and "lib/openzeppelin-contracts":{"Tag":"v4.9.4"} updated to "lib/openzeppelin-contracts":{"Tag":"v5.0.2"}

Maybe on forge update we should print out versions (and should follow up with docs / book update).

(1). Yeah, this is because we run git submodule update --remote as before, and checkout to the correct tag/rev after the updates have been downloaded. EDIT: On second thought, we could remove the --remote flag for deps that specify a tag/rev, this should fix it. Now this is as good as not running update for these deps as --remote flag is only intended for modules pinned to a branch.

(2) Deps can be updated along with the values in foundry.lock like so forge update OpenZeppelin/[email protected] no need to manually checkout any lib or edit forge-submodule-info.json

@yash-atreya yash-atreya marked this pull request as draft December 11, 2024 07:47
@yash-atreya yash-atreya requested a review from mattsse January 30, 2025 12:57
@zerosnacks
Copy link
Member

zerosnacks commented Jan 30, 2025

When running:

forge init counter (commit)

followed by

forge install openzeppelin/openzeppelin-contracts@tag=v4.9.4 the following error is thrown

Installing openzeppelin-contracts in /home/zerosnacks/Projects/counter/lib/openzeppelin-contracts (url: Some("https://github.com/openzeppelin/openzeppelin-contracts"), tag: Some("v4.9.4"))
Error: The target directory is a part of or on its own an already initialized git repository,
and it requires clean working and staging areas, including no untracked files.

Check the current git repository's status with `git status`.
Then, you can track files with `git add ...` and then commit them with `git commit`,
ignore them in the `.gitignore` file, or run this command again with the `--no-commit` flag.

It looks like the foundry.lock file is being written prior to .gitmodules being created which causes this issue

This also means the foundry.lock is added to the repo whilst the .gitmodules addition reverts causing issues when trying to re-run

Side note: it was never really clear to me why we are creating a commit upon adding a dependency

@grandizzy grandizzy added this to the v1.1.0 milestone Feb 4, 2025
@zerosnacks zerosnacks requested a review from DaniPopes February 27, 2025 13:33
@zerosnacks zerosnacks changed the title feat(forge): pin tags/revs for deps feat(dependencies): support pinning of tags / revs with .gitmodules Feb 28, 2025
Comment on lines +156 to +162
/// Writes the lockfile to the project root.
pub fn write(&self) -> Result<()> {
foundry_common::fs::write_json_file(&self.lockfile_path, &self.deps)?;
trace!(at= ?self.lockfile_path, "wrote lockfile");

Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if the lockfile was rendered as pretty formatted JSON, makes it easier to verify the rev's

@zerosnacks
Copy link
Member

introduce a basic lockfile foundry.lock that consists of a mapping from relative lib_path to DepIdentifier. This file is committed with every forge install.

because we don't auto-commit anymore this is no longer true - correct?

@zerosnacks
Copy link
Member

Would be great if this PR was accompanied by an update to the book to document the forge update owner/dep@new-tag behavior as it will be a common question

@zerosnacks zerosnacks self-requested a review February 28, 2025 14:51
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.

Some minor nits, implementation and tests I performed all look good

Updated the title

@zerosnacks zerosnacks changed the title feat(dependencies): support pinning of tags / revs with .gitmodules feat(dependencies): support pinning of tags / revs when using .gitmodules using foundry.lock Feb 28, 2025
@zerosnacks zerosnacks changed the title feat(dependencies): support pinning of tags / revs when using .gitmodules using foundry.lock feat(dependencies): support pinning of tags / revs when using .gitmodules with foundry.lock Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature T-likely-breaking Type: requires changes that can be breaking
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

Bug: forge does not add tag information into gitmodules file on install
5 participants