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

Update remappings and install instructions for Foundry on docs site #4498

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Jul 31, 2023

Fixes #4491.

Simply syncs README.md with index.adoc.

Rendered


I've also changed our remappings.txt because they are used by forge remappings when the user runs their code and we want imports to look the same as with the npm package.

We can't remove the remappings part from the installation instructions yet because for v4.9.3 it still has to be manually added.

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2023

⚠️ No Changeset found

Latest commit: 6e68074

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

docs/modules/ROOT/pages/index.adoc Outdated Show resolved Hide resolved
@frangio frangio merged commit 8643fd4 into OpenZeppelin:master Aug 9, 2023
@frangio frangio deleted the foundry-docs branch August 9, 2023 02:22
@pcaversaccio
Copy link
Contributor

A quick input as I see you're concerned about people pulling the latest master branch. Why not recommend people using a branch in the .gitmodules file like I did here? There are no separate branches for the patch releases, but the minor releases with the latest patch have a dedicated branch for the most important version 4.x releases.

@frangio
Copy link
Contributor Author

frangio commented Aug 9, 2023

The only issue is if you're in 4.6 but the latest is 4.9 you will not get any 4.x patches, because those are only released for the latest minor.

That said running forge update if you don't specify a release branch will get you the development branch so that doesn't work anyway...

What's the process for setting it up like you show there? Is there a forge install command that's possible or do you need to edit .gitmodules manually?

@pcaversaccio
Copy link
Contributor

What's the process for setting it up like you show there? Is there a forge install command that's possible or do you need to edit .gitmodules manually?

Yeah, manually, unfortunately. I mean, it's more like a recommendation for the case where someone invokes git submodule update --remote --recursive and pulls the latest master changes. If you're on the eg 4.9 branch, you would still pull the latest changes locally, but you're still on the correct branch.

@frangio
Copy link
Contributor Author

frangio commented Aug 9, 2023

Does forge update respect what you put in .gitmodules? Is it equivalent to git submodule update?

@pcaversaccio
Copy link
Contributor

That's from the docs:

image

So I did a local test:

~$ forge init
~$ forge install openzeppelin/openzeppelin-contracts
   => Installed openzeppelin-contracts v4.9.3 (same behaviour as invoking `forge install openzeppelin/openzeppelin-contracts@v4.9.3`)
~$ forge update
   => Nothing gets updated
~$ git submodule update
   => Submodule path 'lib/forge-std': checked out '74cfb77e308dd188d2f58864aaf44963ae6b88b1'
   => Submodule path 'lib/openzeppelin-contracts': checked out 'fd81a96f01cc42ef1c9a5399364968d0e07e9e90' (this is the last commit in the `release-v4.9` branch https://github.com/OpenZeppelin/openzeppelin-contracts/commit/fd81a96f01cc42ef1c9a5399364968d0e07e9e90)
~$ git submodule update --remote
   => Submodule path 'lib/forge-std': checked out 'f18e8aa3e72eef83518766eb34ad8c8d8e2aa0aa'
   => Submodule path 'lib/openzeppelin-contracts': checked out '9e3f4d60c581010c4a3979480e07cc7752f124cc' (latest commit on master branch)

After that, I added the following to .gitmodules

[submodule "lib/openzeppelin-contracts"]
	path = lib/openzeppelin-contracts
	url = https://github.com/openzeppelin/openzeppelin-contracts.git
	branch = release-v4.6

and ran forge update (I had to first delete the lib directory in the OZ submodule to make it work due to a warning I got warning: unable to rmdir 'lib/erc4626-tests': Directory not empty). However nothing happened.

TL;DR: forge update does not respect .gitmodules based on my testing, and forge update is technically not equal to git submodule update as forge update targets the tag while git submodule update targets the latest branch commit.

I would like to loop in @Evalir from the Foundry team to confirm this behaviour.

@mds1
Copy link

mds1 commented Aug 11, 2023

TL;DR: forge update does not respect .gitmodules based on my testing

Just to clarify what you mean here, when you added branch = release-v4.6 and ran forge update you said nothing happened, which sounds like forge is respecting gitmodules because there are more recent versions that it didn't update to?

@pcaversaccio
Copy link
Contributor

TL;DR: forge update does not respect .gitmodules based on my testing

Just to clarify what you mean here, when you added branch = release-v4.6 and ran forge update you said nothing happened, which sounds like forge is respecting gitmodules because there are more recent versions that it didn't update to?

But the checked out commits didn't update as expected. I might need to run another test to confirm. What is your understanding of forge update's behaviour in this context?

@mds1
Copy link

mds1 commented Aug 11, 2023

But the checked out commits didn't update as expected.

Oh maybe I misunderstood. Does the release-v4.6 branch move as new patch versions are released, and you originally had it pointing to an older patch and expected forge update to move it to a newer patch version, but it did not move?

What is your understanding of forge update's behaviour in this context?

I'm not sure what forge's expected behavior is here, I'd suggest opening an issue in the forge repo with actual vs. expected behavior. Respecting what's in .gitmodules seems reasonable to me. Submodules are kinda weird/finnicky so I never update them via forge or git, instead I just forge remove then re-forge install the desired version 😅

@pcaversaccio
Copy link
Contributor

Oh maybe I misunderstood. Does the release-v4.6 branch move as new patch versions are released, and you originally had it pointing to an older patch and expected forge update to move it to a newer patch version, but it did not move?

There is only one version for release-v4.6 and no patches so I don't think this was an issue. However, it didn't match the latest commit of the release branch. Anyways, will retest; maybe a mistake on my end.

I'm not sure what forge's expected behavior is here, I'd suggest opening an issue in the forge repo with actual vs. expected behavior. Respecting what's in .gitmodules seems reasonable to me. Submodules are kinda weird/finnicky so I never update them via forge or git, instead I just forge remove then re-forge install the desired version 😅

@frangio what would be your expected behaviour? Before opening a new issue I want to first understand the exact current behaviour of forge update by @Evalir. So a current test what I did is the following: I first used forge init and thereafter I amended the .gitmodules (and did nothing else) with the following entry:

[submodule "lib/openzeppelin-contracts"]
	path = lib/openzeppelin-contracts
	url = https://github.com/openzeppelin/openzeppelin-contracts.git
	branch = release-v4.6

Thereafter, I ran forge install openzeppelin/openzeppelin-contracts and installed the version openzeppelin-contracts v4.9.3 however. That's definitely something I wouldn't expect. Running forge update after will, however, update it accordingly to version v4.6.

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

Successfully merging this pull request may close these issues.

Add directions to install using foundry in documentation
4 participants