Skip to content
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

Multiple rust libraries with submodules #4456

Merged

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Sep 5, 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!

@graydon graydon marked this pull request as draft September 5, 2024 07:41
@graydon graydon force-pushed the multiple-rust-libraries-with-submodules branch from e8ef9b0 to 0e67687 Compare September 5, 2024 22:59
src/rust/src/lib.rs Outdated Show resolved Hide resolved
src/rust/src/lib.rs Outdated Show resolved Hide resolved
@graydon graydon force-pushed the multiple-rust-libraries-with-submodules branch 4 times, most recently from aa191f2 to 4596ffa Compare September 8, 2024 22:06
@graydon graydon force-pushed the multiple-rust-libraries-with-submodules branch from 158c722 to a0afae7 Compare September 11, 2024 05:02
@graydon graydon force-pushed the multiple-rust-libraries-with-submodules branch from db75446 to c134e66 Compare September 18, 2024 08:03
@graydon graydon marked this pull request as ready for review September 19, 2024 01:44
@graydon graydon force-pushed the multiple-rust-libraries-with-submodules branch from e6b5650 to 97866aa Compare September 19, 2024 01:51
Builds/VisualStudio/stellar-core.vcxproj Outdated Show resolved Hide resolved
Cargo.lock Show resolved Hide resolved
@sisuresh sisuresh added this pull request to the merge queue Sep 19, 2024
Merged via the queue into stellar:master with commit 61e8d53 Sep 19, 2024
13 checks passed
@graydon graydon mentioned this pull request Sep 27, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 27, 2024
This makes 3 changes:

1. Stops passing `--features testutils` to soroban when building with
`BUILD_TESTS`
2. Starts passing it to librust_stellar_core.a (where it enables the
side-by-side execution feature)
3. Fixes a couple minor typos in the code that was gated by that

The key part here is the first; evidently having `testutils` turned on
on soroban makes the cost structure observably different and thereby
causes divergence during replay. This seems wrong to me, and something
we should track down and eliminate, but in the meantime this at least
(a) gets rid of that divergence and (b) does what we did before I
accidentally started passing `testutils` in the recent soroban-submodule
change (#4456)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants