-
Notifications
You must be signed in to change notification settings - Fork 970
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
sisuresh
merged 2 commits into
stellar:master
from
graydon:multiple-rust-libraries-with-submodules
Sep 19, 2024
Merged
Multiple rust libraries with submodules #4456
sisuresh
merged 2 commits into
stellar:master
from
graydon:multiple-rust-libraries-with-submodules
Sep 19, 2024
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
graydon
force-pushed
the
multiple-rust-libraries-with-submodules
branch
from
September 5, 2024 22:59
e8ef9b0
to
0e67687
Compare
sisuresh
reviewed
Sep 6, 2024
sisuresh
reviewed
Sep 6, 2024
sisuresh
reviewed
Sep 6, 2024
graydon
force-pushed
the
multiple-rust-libraries-with-submodules
branch
4 times, most recently
from
September 8, 2024 22:06
aa191f2
to
4596ffa
Compare
graydon
force-pushed
the
multiple-rust-libraries-with-submodules
branch
from
September 11, 2024 05:02
158c722
to
a0afae7
Compare
graydon
force-pushed
the
multiple-rust-libraries-with-submodules
branch
from
September 18, 2024 08:03
db75446
to
c134e66
Compare
graydon
force-pushed
the
multiple-rust-libraries-with-submodules
branch
from
September 19, 2024 01:51
e6b5650
to
97866aa
Compare
dmkozh
reviewed
Sep 19, 2024
dmkozh
approved these changes
Sep 19, 2024
Merged
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
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.
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:
Disadvantages:
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:
So instead I've decided to redo this task using a slightly weaker tool:
cargo tree
, which is built-in to cargo (notcargo 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: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 downgradessmallvec 1.13.2 -> 1.10.0
,libm 0.2.8 -> 0.2.7
andwasmparser-nostd 0.100.2 -> 0.100.1
, and the removal ofahash
. 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!