[move][cli] Support safe module republishing with breaking changes ch… #6753
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can't add @mengxu-fb as a reviewer for some reason, but definitely want your opinion! |
meng-xu-cs
reviewed
Nov 25, 2020
language/tools/move-cli/src/main.rs
Outdated
| let deps = self.get_compilation_deps()?; | ||
| let to_compile_filenames = move_lang::find_move_filenames(to_compile, true)?; | ||
| // convert /path/to/File.move to File.move | ||
| let file_names: Vec<&str> = to_compile_filenames.iter().map(|s| get_stem(s)).flatten().collect(); |
There was a problem hiding this comment.
Maybe using a Set here instead of a Vec?
meng-xu-cs
reviewed
Nov 25, 2020
language/tools/move-cli/src/main.rs
Outdated
| // exclude any filenames in `deps` that do not match File.move | ||
| // Note: this is best-effort in at least two ways: | ||
| // (1) It will wrongly filter modules in the same name under different addresses (e.g., 0x1/M.move and 0x2/M.move) | ||
| // (2) It will fail to filter modules with the same filename, but different module names (e.g., 0x1/M in both M1.move and M2.move) |
There was a problem hiding this comment.
(3) It may cause modules to be lost? (e.g., 0x1/M and 0x1/N in the deps' M.move but only 0x1/M in the updated M.move (although I guess this should not be an issue for stdlib modules).
|
Thanks Sam for pinning me here. The changes LGTM, and I'll leave it to others to stamp it :) I think given what the CLI has, the file exclusion can only be best-effort and never perfect. I wish we had compiler-side change to support module updates directly, say, in, |
sblackshear
force-pushed
the
cli_compatibility_checks
branch
4 times, most recently
from
6dcf2c9
to
a1c75d0
Compare
tzakian
approved these changes
Dec 17, 2020
language/tools/move-cli/src/main.rs
Outdated
| @@ -91,6 +93,11 @@ enum Command { | |||
| /// If set, modules will not override existing ones in storage | |||
| #[structopt(long = "no-republish")] | |||
| no_republish: bool, | |||
| /// By default, code that might cause breaking changes for bytecode | |||
| /// linking or data layout compatibility checks will not be published. | |||
| /// Set this flag to ignore breakng changes checks and publish anyway | |||
There was a problem hiding this comment.
nit
Suggested change
| /// Set this flag to ignore breakng changes checks and publish anyway | |
| /// Set this flag to ignore breaking changes checks and publish anyway |
language/tools/move-cli/src/main.rs
Outdated
| let new_api = Module::new(m); | ||
| let compat = Compatibility::check(&old_api, &new_api); | ||
| if !compat.is_fully_compatible() { | ||
| println!("Breaking change detected--publishing aborted. Re-run with --ignore-breaking-changes to publish anyway.") |
There was a problem hiding this comment.
Possibly a larger question for the CLI as well, but I'm thinking this should probably be an eprintln! instead since this is a warning/error.
There was a problem hiding this comment.
Yeah, good call. I'll do that for now, and in the future I'd like to get rid of all print's and turn them into writes to a buffer. I think we need to do this in order to run CLI tests concurrently in the same process.
sblackshear
force-pushed
the
cli_compatibility_checks
branch
from
a1c75d0
to
49119d5
Compare
|
/land |
bors-libra
pushed a commit
that referenced
this pull request
Dec 17, 2020
…ecks - If the CLI is trying to compile `M.move`, the logic for finding compilation deps now removes `M.move` from the deps list. More generally, any files directly or transitively reachable from the compilation targets are removed from the deps list. This fixes the annoying "duplicate definition" warning that you used to get when trying to re-publish a module - Before a `move publish`, the CLI runs the bytecode linking and data layout compatibility checks and reports an error if the code to be published could cause a breaking change to either linking or data layout - Added tests for republishing, running `move check M.move` after publishing `M.move`, and the breaking changes checks Closes: #6753
|
Cluster Test Result ❗ Cluster Test failed - non-zero exit code for Repro cmd: |
|
💔 Test Failed - Build images and run cluster test |
|
/land |
|
/land |
|
@sblackshear 💡 This PR is already queued for landing |
sblackshear
added a commit
to sblackshear/libra
that referenced
this pull request
Dec 17, 2020
This command does a bunch of sanity checks to ensure that the storage directory is well-formed. For now: - All modules verify - All modules link - All resources deserialize w.r.t the current version of their declaring module It does this in the obvious way: iterating through the global storage, collecting each module/resource, and doing the check. This is useful for detecting breaking changes (both code and data layout) after-the-fact, as well as bugs in the CLI. I was motivated to create this check when I noticed a bug where `move publish` sometimes publishes a module `M` without publishing library modules. If you run `move doctor` on such a state, it would catch the issue. I am planning to add `move doctor` to a bunch of the publish tests once I fix this bug. It will also be useful for testing the `--ignore-breaking-changes` flag in diem#6753.
sblackshear
added a commit
to sblackshear/libra
that referenced
this pull request
Dec 18, 2020
This command does a bunch of sanity checks to ensure that the storage directory is well-formed. For now: - All modules verify - All modules link - All resources deserialize w.r.t the current version of their declaring module It does this in the obvious way: iterating through the global storage, collecting each module/resource, and doing the check. This is useful for detecting breaking changes (both code and data layout) after-the-fact, as well as bugs in the CLI. I was motivated to create this check when I noticed a bug where `move publish` sometimes publishes a module `M` without publishing library modules. If you run `move doctor` on such a state, it would catch the issue. I am planning to add `move doctor` to a bunch of the publish tests once I fix this bug. It will also be useful for testing the `--ignore-breaking-changes` flag in diem#6753.
This was referenced Nov 10, 2023
This was referenced Nov 17, 2023
This was referenced Nov 25, 2023
This was referenced Dec 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ecks
M.move, the logic for finding compilation deps now removesM.movefrom the deps list.More generally, any files directly or transitively reachable from the compilation targets are removed from the deps list. This
fixes the annoying "duplicate definition" warning that you used to get when trying to re-publish a module
move publish, the CLI runs the bytecode linking and data layout compatibility checks and reports an error if thecode to be published could cause a breaking change to either linking or data layout
move check M.moveafter publishingM.move, and the breaking changes checks