move: build writes toolchain version to Move.lock #15166
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
bdc5127
to
a72504d
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
a72504d
to
21a7f7c
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
21a7f7c
to
8f5c108
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
8f5c108
to
bac375b
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
bac375b
to
109354a
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
109354a
to
80032ca
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
80032ca
to
551fb17
Compare
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
551fb17
to
11717f2
Compare
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "sui-move-build" | |||
| version = "0.0.0" | |||
| version.workspace = true | |||
There was a problem hiding this comment.
Exposes the binary / root crate version in env!("CARGO_PKG_VERSION")
| if modified || install_dir_set { | ||
| // (1) Write the Move.lock file if the existing one is `modified`, or | ||
| // (2) `install_dir` is set explicitly, which may be a different directory, and where a Move.lock does not exist yet. |
There was a problem hiding this comment.
This addresses what I consider a bug: this block writes Move.lock to install_dir when it is modified. If install_dir is set to something other than the current directory (let's say /tmp/foo) and run build, then Move.lock would only be written to install_dir if Move.lock changed. So /tmp/foo would be empty if we build again and a Move.lock exists and is the same as before, but if Move.lock does not exist or changed, then it would be written to /tmp/foo.
We should be writing to /tmp/foo if it install_dir is set (i.e., ensure Move.lock is written when we're not on the default package path). Tests helpfully rely on checking the Move.lock file in install_dir (1) so if the Move.lock doesn't exist there, things break.
There was a problem hiding this comment.
Reasonable! We should arguably be also looking in the install dir for the lock file that DependencyGraph reads as well, to completely fix this bug, but the difference in behaviour in what you have here vs that is likely an un-exercised edge case, so all good.
| file.set_len(0)?; | ||
| file.rewind()?; |
There was a problem hiding this comment.
Small fix: we need to truncate the file to erase all previous content when updating these values, now covered by test.
| if let Ok(ref compiled) = result { | ||
| compiled | ||
| .package | ||
| .compiled_package_info | ||
| .build_flags | ||
| .update_lock_file_toolchain_version(&path, env!("CARGO_PKG_VERSION").into()) | ||
| .map_err(|e| SuiError::ModuleBuildFailure { | ||
| error: format!("Failed to update Move.lock toolchain version: {e}"), | ||
| })?; | ||
| } |
There was a problem hiding this comment.
Main change.
This is the best place to write lock file logic right now. It is the top level function for source verification and should have all the context we care about before building.
I say "right now" because I'd love to flatten things and have these build steps happen in the external-crates move-package, in one place, which we can do at a later stage.
rvantonder
changed the title
| if modified || install_dir_set { | ||
| // (1) Write the Move.lock file if the existing one is `modified`, or | ||
| // (2) `install_dir` is set explicitly, which may be a different directory, and where a Move.lock does not exist yet. |
There was a problem hiding this comment.
Reasonable! We should arguably be also looking in the install dir for the lock file that DependencyGraph reads as well, to completely fix this bug, but the difference in behaviour in what you have here vs that is likely an un-exercised edge case, so all good.
rvantonder
force-pushed
the
rvt/write-lock-file-3
branch
from
11717f2
to
b9306f2
Compare
## Description As in title, see inline for more. ## Test Plan Updated tests. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [x] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes - The `Move.lock` file will update with `[move.toolchain-version]` fields when building a package (e.g., with `sui move build`). Please check in and commit changes to the `Move.lock` file as usual.
Description
As in title, see inline for more.
Test Plan
Updated tests.
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
Toolchain version is now written to a package's Move.lock file on build.