-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[move] Use workspace dependencies for third-party/move #15461
Conversation
⏱️ 1h 35m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c684964
to
b020c0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I added a comment in places where I thought we could use "workspace = true", but there could be good reasons why you left them as is (but it was not obvious to me). I realized the comments applied to the same small set of packages in various places, so if you justify them once per unique package, that should suffice.
move-cli = { path = "../../tools/move-cli" } | ||
move-package = { path = "../../tools/move-package" } | ||
move-cli = { workspace = true } | ||
move-package = { workspace = true } | ||
move-stdlib = { path = "../../move-stdlib", features = ["testing"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be made { workspace = true }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For stdlib, I am not sure mostly because we have 2 of those😄 one we use for Aptos (as a workspace dependency aptos-move-stdlib
), and one defined in third-party move. I think the reason for this is gas, and we want to move some of gas infra (cc @vgao1996 to confirm my thinking) to third party to be able to use it directly. I did not add it as a workspace dependency to avoid confusion between the two.
third_party/move/move-bytecode-verifier/bytecode-verifier-tests/Cargo.toml
Outdated
Show resolved
Hide resolved
move-stdlib = { path = "../../move-stdlib", features = ["testing"] } | ||
move-symbol-pool = { path = "../../move-symbol-pool" } | ||
move-symbol-pool = { workspace = true } | ||
move-table-extension = { path = "../../extensions/move-table-extension" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be made { workspace = true }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is the same as for move-stdlib
. We have a copy of table extension in our aptos tree due to gas charging (aptos-move-table-extension
or aptos-table-extension
iirc), and so this one is actually not used, so we should not add it to the workspace to avoid having two extensions :)
b020c0c
to
e88cfa2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
We have been using workspace vs path dependencies for third_party/move pretty randomly, ususally copy pasting them. This PR scans through all cargo dependencies and ensures that
The goal is to make sure syncing with
move-on-aptos
repo is easy: when adding new crates and dependencies we would only need to modify the rootCargo.toml
there to add a new create/dependency. Right now, this is not possible.How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist