Figure out how to bump rust dependencies without affecting the prev dependency tree #4278
Assignees
Comments
github-merge-queue bot
pushed a commit
that referenced
this issue
Sep 19, 2024
This modifies the approach we use to linking multiple sorobans. The new approach builds each soroban separately into its own `rlib` using a `--locked` cargo build, followed by manually providing them as `--extern` definitions to the top-level rust build of libstellar_core.rlib. It is an approach to solving problems of cargo solving/merging/advancing dependency versions when doing soroban multi-versioning (a.k.a. #4278). The approach here is deeply indebted to @leighmcculloch -- he both had the initial idea and overcame almost every obstacle I encountered along the way. I am just the automake-wrestling keyboard monkey in this PR. Advantages: - We get the soroban lockfiles that were tested upstream, no more no less. - We get to use git submodules to just point directly to the soroban source trees. - Cargo treats all the builds separately: each soroban, then stellar-core itself. Never merges deps of any of them. Disadvantages: - Submodules make a lot of people sad - The necessary automake code _definitely_ makes me sad - It breaks vscode or any other IDE trying to edit contract.rs (which, granted, is only 500 lines of code) ## Dep-tree checking There's some existing machinery in stellar-core that bakes-in the Cargo.lock file and then compares the dep-tree of each soroban host in it to fixed (manually maintained) dep-tree files we generate with the `cargo lock tree` cargo-extension. This machinery no longer works with this new scheme: - There is no single lockfile anymore - The lockfiles in the submodules contain lots of additional deps and churn (dev-deps especially) So instead I've decided to redo this task using a _slightly_ weaker tool: `cargo tree`, which is built-in to cargo (not `cargo lock tree`). This loses some precision (`cargo tree` only outputs package version numbers, not checksums) but it allows us to specify the features, and exclude the dev-deps, of each submodule. Along the way I've changed it from a dynamic check to a static one: the build just won't succeed if the expected deptrees (checked-in to the stellar-core repo) don't match the actual ones (extracted at build time from the submodules). The resulting errors look like this: ``` --- rust/src/dep-trees/p21-expect.txt 2024-09-07 20:38:22.056593002 -0700 +++ ../src/rust/src/dep-trees/p21-actual.txt 2024-09-07 20:38:11.852700915 -0700 @@ -5,7 +5,7 @@ │ ├── curve25519-dalek-derive v0.1.0 (proc-macro) │ │ ├── proc-macro2 v1.0.69 │ │ │ └── unicode-ident v1.0.9 -│ │ ├── quote v1.0.32 +│ │ ├── quote v1.0.33 │ │ │ └── proc-macro2 v1.0.69 (*) │ │ └── syn v2.0.39 │ │ ├── proc-macro2 v1.0.69 (*) dep trees differ, please update p21-expect.txt or roll back submodule ``` I think this is a generally superior developer experience for us, despite the minor loss in precision around dep identities. In practice I think the package version numbers are precise enough. ## Dep-tree differences If you take a look at the dep tree being checked in with this change for the p21 host and compare to the dep tree baked into master's current lockfile for the `[email protected]` package, you will see some slight differences: specifically you'll see that this PR _downgrades_ `smallvec 1.13.2 -> 1.10.0`, `libm 0.2.8 -> 0.2.7` and `wasmparser-nostd 0.100.2 -> 0.100.1`, and the removal of `ahash`. These downgrades are actually a _revert_ of changes that happened recently in e967b18 where I generalized support for multiple versions of soroban and simultaneously brought the p22 env into core: at that point I was forced (by cargo's aggressive version unification) to allow those _upgrades_ to the dep-tree of the p21 host, even though I kinda didn't want to. I accepted them at the time as "probably unobservable and worth the bet" but, in fact, the presence of such unwanted upgrades was one of the motivating factors for _this PR_, that reverts them by downgrading them. Luckily we have not _released_ anything with those unwanted-upgrades yet, so reverting and downgrading them to the exact versions that (a) shipped in p21 and (b) are baked into the lockfile of the env git repo at the point in history when p21 was released is the right thing to do here. But it was good to check!
|
Fixed by the sub-module approach in core. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
and others
For example, core uses ecdsa 0.16.7, and env used 0.16.8. This was fine until the secp256 change, when soroban-env-host took a direct dependency on the ecdsa crate (among others that have the same issue). We now want the core lock file to specify both ecdsa 0.16.7 (for prev env) and 0.16.8 (for curr env), but cargo only allows duplicate crates if the major version is different, or in this case where it's zero, the minor versions need to be different.
We don't want to update the dependencies for the current protocols host, so we ended up pinning the env version to what was specified in the lock file, but this prevents us from bumping dependencies.
The text was updated successfully, but these errors were encountered: