-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[packages] Modify dependency graph construction to use lock files #12575
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
2b0175b
to
03c07ed
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.
Pausing my review to share some of the initial comments! Overall looks good, but it's a lot to wrap ones head around in one sitting. I'm having a think about whether there's a way to get around rolling our own cycle detection, etc, but I'm leaning towards "maybe not".
external-crates/move/tools/move-package/tests/test_sources/one_dep_wrong_name/Move.resolved
Outdated
Show resolved
Hide resolved
...tes/move/tools/move-package/tests/test_sources/external_overlap_fail_symmetric/Move.resolved
Outdated
Show resolved
Hide resolved
...tes/move/tools/move-package/tests/test_sources/external_overlap_fail_symmetric/Move.resolved
Outdated
Show resolved
Hide resolved
.../move-package/tests/test_sources/diamond_problem_dep_conflicting_dep_reg_overrides/Move.toml
Outdated
Show resolved
Hide resolved
...e-package/tests/test_sources/diamond_problem_dep_conflicting_dep_reg_overrides/Move.resolved
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
// bail if encountering external dependency that would require running the external | ||
// resolver | ||
// TODO: should we consider handling this here? | ||
PM::Dependency::External(_) => return Ok(None), |
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.
The external resolver produces a lock file itself, so you could consider hashing the output of running the resolver, rather than bailing out here.
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.
(To do this efficiently would require running the external resolvers up-front and saving their outputs).
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.
We can treat this as a follow-up.
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
6b72398
to
da9f58e
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.
The new implementation is definitely looking easier to follow, thanks for doing that! I think there might be some issues with various edge cases in that implementation though -- I've pointed out a couple, LMKWYT.
external-crates/move/tools/move-package/src/resolution/lock_file/schema.rs
Outdated
Show resolved
Hide resolved
...e-package/tests/test_sources/diamond_problem_dep_conflicting_dep_reg_overrides/Move.resolved
Outdated
Show resolved
Hide resolved
...-package/tests/test_sources/diamond_problem_dep_external_incorrect_override_v1/Move.resolved
Outdated
Show resolved
Hide resolved
...-package/tests/test_sources/diamond_problem_dep_external_incorrect_override_v2/Move.resolved
Outdated
Show resolved
Hide resolved
manifest_digest: Some( | ||
"40ECD99EA24DDE388D25DBAB6EFAAF04543D55F0AD108BA6F489B0C71BB618AB", | ||
), | ||
deps_digest: None, |
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.
Is there a test case where deps_digest
is not None
? (i.e. where all the deps have lock files?). If not, we should add one.
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.
I added the test (multiple_deps_from_lock
), and the graph gets generated properly (as evidenced by the Move.resolved
file) but the Move.locked
file. I wonder if it has something to do with the testing harness and tried to debug it but without much success so far - if some quick idea pops up, please share!
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
for pkg_name in all_packges { | ||
let mut existing_pkg_info: Option<(Symbol, &DependencyGraph, &Package)> = None; | ||
for (dep_name, (g, _)) in &dep_graphs { | ||
if let Some(pkg) = g.package_table.get(&pkg_name) { |
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.
Can you include self
among the dep_graphs
and include the immediate dependencies among all_packages
to avoid having to special case them?
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.
I am not sure I understand this particular request... The self
graph is initially empty and it's done on purpose so that when we resolve conflicts, we know which of the sub-graphs are conflicting (before, when we compared a sub-graph to a partially merged combined graph, we were loosing this information). Are you suggesting we pre-populate the self
graph with some data before this loop starts? I am likely missing something here - some more info would be greatly appreciated!
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.
If this is doable, it would simplify and resolve a few of the comments I've made on potential issues with special cases below.
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.
Sorry for some reason when I reviewed this last, I didn't see your reply! Not sure what happened there, but yes, what I was suggesting was for the self graph to be pre-populated with the edges to its immediate dependencies, and then subsequently included in the merge.
e425741
to
ef838a3
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.
These errors are much much clearer, thank you for working on that.
In the new tests, I spotted some cases that don't look right (the resolution from lock file that's missing packages, and the address substitution difference that's treated as an error), plus some clean-ups etc, but this is now the home stretch.
...tes/move/tools/move-package/tests/test_sources/multiple_deps_from_lock_no_manifest/Move.toml
Outdated
Show resolved
Hide resolved
...move/tools/move-package/tests/test_sources/diamond_problem_dep_nested_dep_conflict/Move.toml
Show resolved
Hide resolved
...s/move-package/tests/test_sources/diamond_problem_dep_incorrect_override_empty/Move.resolved
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
let PM::InternalDependency { | ||
kind, | ||
version, | ||
subst, | ||
digest, | ||
dep_override, | ||
} = internal; |
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.
nit, inline into match
pattern arm?
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.
Some unpacked fields are also used when creating Dependency
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.
Not sure I follow on this one, I'm suggesting that instead of
PM::Dependency::Internal(internal) => {
let PM::InternalDependency {
/* ... */
} = internal;
You have:
PM::Dependency::Internal(PM::InternalDependency {
/* ... */
}) => {
Because I don't see any other reference to internal
in the rest of the body. I didn't think the fact that the unpacked fields are used to create a Dependency
would make a difference here but maybe I'm missing something.
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.
Unpacking of internal
defines a bunch of local variables (e.g., subst
) that are used outside of the match
pattern.
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.
Maybe I'm missing something super obvious, if so apologies, but to me, the body of this function looks like this:
fn insert_direct_dep(...) {
match dep {
Dependency::Internal(internal) => {
let PM::InternalDependency {
... (1)
} = internal;
... (2)
}
Dependency::External(_) => {
...
}
}
}
Where all the references to variables bound in (1) are only used in (2) (i.e. there's literally nowhere else you could refer to those bindings), so the suggestion to turn this structure into:
fn insert_direct_dep(...) {
match dep {
Dependency::Internal(PM::InternalDependency { ... (1) }) => {
... (2)
}
Dependency::External(_) => {
...
}
}
}
should be fine?
Again, entirely possible that I'm missing something obvious.
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.
I totally misunderstood what you were suggesting :-) and now that I do, I made the change.
For the record, there was another match in this function (now replaced with if let Entry::Vacant(entry)...
) and I somehow thought you suggested moving struct unpack inside this match statement...
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
for pkg_name in all_packges { | ||
let mut existing_pkg_info: Option<(Symbol, &DependencyGraph, &Package)> = None; | ||
for (dep_name, (g, _)) in &dep_graphs { | ||
if let Some(pkg) = g.package_table.get(&pkg_name) { |
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.
If this is doable, it would simplify and resolve a few of the comments I've made on potential issues with special cases below.
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Show resolved
Hide resolved
a991869
to
4bc78a8
Compare
86b8274
to
642e03a
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.
🎉 Let's go!!!! This is good to go, thanks for working on this @awelc !
external-crates/move/tools/move-package/tests/test_sources/nested_pruned_override/Move.toml
Show resolved
Hide resolved
&self, | ||
dep_name: Symbol, | ||
g: &'a DependencyGraph, | ||
dep_graphs: &'a BTreeMap<PM::PackageName, (DependencyGraph, DependencyMode, bool, bool)>, |
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.
nit: The value type is verging on needing a struct
for clarity, for me.
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.
Good idea!
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
## Description Adding protocol change highlight to release notes script ## Test Plan ``` eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release Sui Protocol Version in this release: XX #13124: Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification. #13417: When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning. #12989: All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs. Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed. #13194: When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects. #12575: The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information. #12933: Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified. #13312: Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm. ```
## Description Adding protocol change highlight to release notes script ## Test Plan ``` eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release Sui Protocol Version in this release: XX #13124: Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification. #13417: When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning. #12989: All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs. Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed. #13194: When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects. #12575: The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information. #12933: Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified. #13312: Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm. ```
…es (#13487) ## Description This is a follow-up to #12575 where making manifest and dependency digests mandatory was left for future work (#12575 (comment)) One issue here is that now external resolvers must also output content for both of these fields (though digests can actually be empty). ## Test Plan All existing tests must pass after the mandatory field adjustment. --- 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 Manifest file digest and dependency digest fields in Move.lock files are are now mandatory.
…es (#13487) ## Description This is a follow-up to MystenLabs/sui#12575 where making manifest and dependency digests mandatory was left for future work (MystenLabs/sui#12575 (comment)) One issue here is that now external resolvers must also output content for both of these fields (though digests can actually be empty). ## Test Plan All existing tests must pass after the mandatory field adjustment. --- 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 Manifest file digest and dependency digest fields in Move.lock files are are now mandatory.
Description
The current implementation of dependency resolution has grown incredibly complex to handle dependency overrides and merging between multiple resolvers. We can simplify the implementation by relying on the lock files of immediate dependencies to gather transitive dependency information. This is maintenance work that will simplify any other changes to this code path.
Test Plan
Adjusted existing tests to the new algorithm.
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
The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information.