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

CHORE: Workspace level Cargo.toml #2

Merged
merged 9 commits into from
Dec 19, 2023
Merged

CHORE: Workspace level Cargo.toml #2

merged 9 commits into from
Dec 19, 2023

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 18, 2023

Takes some of the changes done in db0002c to make all Cargo.toml files take the dependency versions from a common workspace level one.

What this PR doesn't do is try to change any of the dependency versions. I think we should fight that battle after we have established a working CI pipeline which allows us to work with this repository with confidence.

The PR also doesn't try to reorganise the imported subtree structures, e.g. it leaves them as ipc/ipc and fendermint/fendermint, in the hope that this will make it easier to pull in changes that are still actively happening in those repositories.

#1 started updating dependencies but I would recommend that we rebase that on top of this and refrain from rearranging the code layout until this repository is stable and we stop working on the originals.

Testing

A top level make test delegates to all sub-repos, each having their own make test target focusing on their own packages, excluding everything else.

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 18, 2023

@adlrocha when I look at the original contracts I see symlinks (or submodules?): https://github.com/consensus-shipyard/ipc-solidity-actors/tree/dev/lib

However in this repo these are not pointing anywhere useful: https://github.com/consensus-shipyard/ipc-monorepo/tree/main/contracts/lib

Possibly because you created the subtrees from your local filesystem instead of pulling in the remote repositories. Do you have an idea how to fix it? I don't know how these libraries were added in the first place.

Currently when I try to run tests I get this:

cd contracts

  ~/projects/consensuslab/ipc-monorepo/contracts  workspace-toml ···················································································································· ⬢ 20.9.0  1.73.0 22:06:24make test
forge test -vvv --ffi
[⠊] Compiling...
Error: 
Failed to resolve file: "/home/aakoshh/projects/consensuslab/ipc-monorepo/contracts/lib/openzeppelin-contracts/contracts/utils/structs/EnumerableSet.sol": No such file or directory (os error 2).
 Check configured remappings..
    --> "/home/aakoshh/projects/consensuslab/ipc-monorepo/contracts/src/gateway/GatewayGetterFacet.sol"
        "openzeppelin-contracts/utils/structs/EnumerableSet.sol"
make: *** [Makefile:50: test] Error 1

@adlrocha
Copy link
Contributor

@aakoshh, usually we use https://book.getfoundry.sh/projects/dependencies#adding-a-dependency to install dependencies. I tried but I get an error flagging that my commit history is dirty (which is not the case). I also tried git submodule update --init --recursive and it says that there's no url found for the submodules (which conflicts with the message above) :( I'll keep tinkering, but to keep you up to date. What forge install does is basically bring the code to the repo as a submodule.

Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

LGTM. Left a few comments with reminders, but looks good. I'll try to find how we can pull the Solidity dependencies in the meantime (which is the last thing missing)

"ipc/ipc/provider",
"ipc/ipc/sdk",
# Tests seem to be obsolete (using IPC agent):
# "ipc/ipc/testing/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove them. We were waiting for M2.5 to deprecate them, and now we can. I actually removed them from my monorepo PR already.

Suggested change
# "ipc/ipc/testing/*",
# "ipc/ipc/testing/*",

Cargo.toml Outdated
"contracts/binding",
# fvm-utils,
"fvm-utils/primitives",
"fvm-utils/runtime",
Copy link
Contributor

Choose a reason for hiding this comment

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

As part of the monorepo I would like to deprecate fvm-utils. Now that we don't need the ffi and we use solidity contracts the repo is no longer needed and we can move the primitives to the monorepo directly.

Suggested change
"fvm-utils/runtime",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I saw that primitives have been moved, just wanted to defer that after it's all done.

git subtree add -P ipld-resolver ../ipc-ipld-resolver main
```

TODO: Add examples of pulling updates from the upstream repos.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the command to pull changes so you don't have to look for it:

git subtree pull --prefix external/<component> <component> dev

Source: https://stackoverflow.com/questions/45222656/how-do-i-add-a-subtree-to-an-existing-prefix-in-a-new-clone

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 19, 2023

@adlrocha thanks for fixing the submodules!

When I pull and run make test I get this:

Failed to resolve file: "/home/aakoshh/projects/consensuslab/ipc-monorepo/contracts/lib/forge-std/src/Test.sol": No such file or directory (os error 2).
 Check configured remappings..
    --> "/home/aakoshh/projects/consensuslab/ipc-monorepo/contracts/test/AccountHelper.t.sol"
        "forge-std/Test.sol"

Is there something to be done before?

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 19, 2023

I did the following:

  1. remove forge-std from .gitmodules
  2. cd contracts/lib; git submodule add https://github.com/foundry-rs/forge-std
  3. git submodule update --init --recursive

This fixed running the tests, although they fail for different reasons.

I'm not sure if the 3rd step is a required step for checkout.

@aakoshh aakoshh merged commit d18057e into main Dec 19, 2023
@aakoshh aakoshh deleted the workspace-toml branch December 19, 2023 09:54
maciejwitowski pushed a commit that referenced this pull request May 28, 2024
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.

2 participants