[move] Fix toml issues with move-execution #17130
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
|
@amnn or @dariorussi, do you know if I need to update any scripts for execution cuts to reflect these changes in the future? (though we will need to fix the issues with the stdlib before changing any such scripts) |
Cargo.toml
Outdated
There was a problem hiding this comment.
I'm assuming/hoping this was done because you flattened v0 as well to make it easier to make other fixes (rather than it having always been broken), because this should be covered by tests...
external-crates/move/Cargo.toml
Outdated
| @@ -155,6 +158,23 @@ move-vm-test-utils = { path = "crates/move-vm-test-utils" } | |||
| move-vm-types = { path = "crates/move-vm-types" } | |||
| prover_bytecode = { path = "crates/move-stackless-bytecode", package="move-stackless-bytecode" } | |||
|
|
|||
| # v0 | |||
There was a problem hiding this comment.
You don't want to make these workspace deps. Instead, we should follow the pattern that we use for other versioned crates, which is to update the toml files of the packages that depend on them to refer to them at their versioned path, but their unversioned name.
The reason for this is that it's much easier to automate (the script doesn't need to look at rust source files and find all references to the latest version and rename that to the versioned form).
See for example:
external-crates/move/Cargo.toml
Outdated
| "move-execution/v0/crates/*", | ||
| "move-execution/v1/crates/*", | ||
| "move-execution/v2/crates/*", |
There was a problem hiding this comment.
Automating this is going to be ...tricky. The cut tool figures out which packages should be included or excluded from the root workspace here:
sui/sui-execution/cut/src/plan.rs
Line 406 in 127f2cb
But it doesn't currently understand how to handle multiple workspaces, ...or glob patterns. One thing you could do is piggyback off the logic we have in the wrapper script, that updates TOML files when a new cut is introduced, here:
sui/scripts/execution_layer.py
Line 420 in 127f2cb
This looks for comments in TOML files that contain "$CUT" and adds a line above them with "$CUT" replaced with the cut name, see e.g.
Lines 18 to 32 in 127f2cb
This is maybe a little more brittle, but I think it's the fastest way to get unblocked without losing the automation support.
There was a problem hiding this comment.
Two comments here: I'm guessing that this crate is not included in the list of crates to cut right now, here:
sui/scripts/execution_layer.py
Lines 354 to 366 in 127f2cb
But we should do that to make sure all future cuts get this crate and it's properly fixed up (Adding a new -p flag with the crate name should suffice).
Also as per the earlier comment about its dependencies, this is where we would need to change the move-bytecode-verifier dependency from a workspae dependency to a path dependency with a customised package name (see my earlier comment on the workspace Cargo.toml).
There was a problem hiding this comment.
...and then all the source files can be left alone.
|
@amnn thanks for all the guidance! |
There was a problem hiding this comment.
Is it just luck of the draw that this is considered a file addition instead of a copy?
There was a problem hiding this comment.
I tried to do git mv with then restoring the previous file. This was the result :(
Not sure if there is anything else to do
There was a problem hiding this comment.
In this file, the changes to members is correct, but I don't think you should need the changes to workspace dependencies -- generally, if a crate is versioned in that way, we other crates should depend on it by path, or through some multiplexing crate (which we don't have within the Move workspace, but for sui it's the sui-execution crate).
There was a problem hiding this comment.
I tried to remove them at one point, but it did not work. Let me try again now that things are all good across the board
There was a problem hiding this comment.
I can take a look tomorrow as well, thanks for these fixes!
There was a problem hiding this comment.
(...never mind, I see it's fixed! Sorry, got confused by Github)
Description
external-crates/move/Cargo.toml, leading to all sorts of issuesTest plan
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.