ping/rust: Refactor into multiple binaries #72

thomaseizinger · 2022-11-08T05:40:33Z

By having one Rust binary per version, we can vary the actual binary from version to version and f.e. fix deprecated API calls. It does introduce a bit of duplication between the different versions but I'd rather have that then not being able to adapt the tests to new APIs.

Instead of activating a feature per libp2p version, we add them all as dependencies. This ensures all transitive dependencies are properly tracked in Cargo.lock. Additionally, this gives us a single place we are can activate all the feature.

For master and pull-request builds, we replace the git target or rev with the one coming from the CI build. Once we trigger a build, cargo will update and resolve the necessary dependencies before that, thus fixing problems such as libp2p/rust-libp2p#2972.

Fixes #71.

mxinden

I like the direction this is taking. Thanks @thomaseizinger!

I will take a more in-depth look.

Mentioning it here to not forget, this pull request also needs to update the root README.md.

mxinden

Changes look good to me. Before we merge, @thomaseizinger can you confirm that this fixes libp2p/rust-libp2p#2972?

ping/_compositions/go-rust-interop-latest.toml

thomaseizinger · 2022-11-14T12:41:11Z

Changes look good to me. Before we merge, @thomaseizinger can you confirm that this fixes libp2p/rust-libp2p#2972?

I'll have to check that locally!

thomaseizinger · 2022-11-15T08:06:52Z

Changes look good to me. Before we merge, @thomaseizinger can you confirm that this fixes libp2p/rust-libp2p#2972?

I'll have to check that locally!

I had to make several changes to make all the usecases work but I've verified it locally now and it all works. I made some more changes to the Dockerfile that should hopefully make it more maintainable.

Pretty much all the configuration is in Cargo.toml now. It behaves like a normal cargo project and you can verify the build easily locally.

When we release a new version, we only need to append an entry to [[groups]] and add a new binary with the next version.

thomaseizinger · 2022-11-15T08:09:42Z

We should also get much better build-performance out of the new Dockerfile, assuming testground has enough space to cache our docker layers (spoiler: they are massive, ~ 20GB).

All released versions should always be pre-build and not require and changes.
To speed up builds for master and PRs, we can occasionally bump the revision that is in the manifest file to a more recent one.

thomaseizinger · 2022-11-15T09:35:11Z

I don't understand why the build is failing: https://github.com/libp2p/test-plans/actions/runs/3468917140/jobs/5795285685#step:6:326

For some reason, testground is building an old Dockerfile.

mxinden · 2022-11-15T14:05:13Z

ping/rust/Dockerfile

-
-RUN mv /tmp/Cargo.toml /tmp/Cargo.lock ./plan
+ARG GIT_TARGET=""
+RUN if [ ! -z "${GIT_TARGET}" ]; then export ESCAPED_GIT_TARGET="$(printf '%s' "${GIT_TARGET}" | sed -e 's/[\/]/\\&/g')"; sed -i "s/^git.*/git = \"https:\/\/${ESCAPED_GIT_TARGET}\"/" ./plan/Cargo.toml; fi # URLs contain forward slashes that need to be escaped for `sed` to work correctly.


As an aside, you can use a different delimiter than / with sed, e.g. #.

See https://stackoverflow.com/a/5864155/4011134

My god, knowing this would have saved me quite some time 😂

I'll try to simplify that then.

mxinden · 2022-11-15T14:08:04Z

I don't understand why the build is failing: https://github.com/libp2p/test-plans/actions/runs/3468917140/jobs/5795285685#step:6:326

For some reason, testground is building an old Dockerfile.

Just a thought: Is the environment variable properly expanded, or accidentally used in plaintext?

Updating git repository `[https://github.com/libp2p/rust-libp2p`](https://github.com/libp2p/rust-libp2p%60)
error: failed to get `libp2p` as a dependency of package `testplan v0.1.0 (/usr/src/testplan/plan)`

Caused by:
  failed to load source for dependency `libp2p`

Caused by:
  Unable to update https://github.com/libp2p/rust-libp2p?rev=$.Env.GitTarget

thomaseizinger · 2022-11-15T20:06:45Z

I don't understand why the build is failing: https://github.com/libp2p/test-plans/actions/runs/3468917140/jobs/5795285685#step:6:326

For some reason, testground is building an old Dockerfile.

Just a thought: Is the environment variable properly expanded, or accidentally used in plaintext?
Updating git repository `[https://github.com/libp2p/rust-libp2p`](https://github.com/libp2p/rust-libp2p%60)
error: failed to get `libp2p` as a dependency of package `testplan v0.1.0 (/usr/src/testplan/plan)`

Caused by:
  failed to load source for dependency `libp2p`

Caused by:
  Unable to update https://github.com/libp2p/rust-libp2p?rev=$.Env.GitTarget

It was when I tested it locally!

ping/rust/Dockerfile

thomaseizinger · 2022-11-16T23:54:50Z

Oh my god it is finally working! 🎉

thomaseizinger · 2022-11-17T00:11:29Z

Okay, this is ready to go in from my end!

Tagging other maintainers for visibility. cc @jxs @elenaf9

thomaseizinger · 2022-11-17T00:30:40Z

It seems that the initial docker layers are not cached between CI runs: https://github.com/libp2p/test-plans/actions/runs/3484164725/jobs/5828413991#step:6:46

I think that was the case before as well though, so this doesn't make things worse I think.

mxinden · 2022-11-17T10:47:18Z

ping/rust/Dockerfile

@@ -4,43 +4,23 @@ WORKDIR /usr/src/testplan
 RUN apt-get update && apt-get install -y cmake protobuf-compiler


@laurentsenta tagging you here for visibility, given that you have worked on the caching optimizations below.

With this pull request cargo build is the same across rust-libp2p versions (apart from master and a specific git commit hash), i.e. it builds all rust-libp2p versions and can thus be reused when testing different rust-libp2p versions. This results in the below simplifications.

Let us know in case you have any objections.

@mxinden thanks for the heads up; no objection on my side, but a question and a comment:

(q) it seems like we "just" moved the version indirection from feature flags to one layer up, but no big change in the Cargo.toml:

So we replace cargo build --features="${VERSION_ID}" with cargo build --bin=${VERSION_ID}

That might be something that comes up again, how is this improving how we track dependencies in the Cargo.lock?

(c) We're dropping dependency caching, i.e. "do not download and rebuild all my dependencies when I change one line in my main.rs".

I added this to make the experience of iterating on the test code more bearable. We might want to add back that feature eventually (through dummy mains, cargo-chef, or something else).

@mxinden thanks for the heads up; no objection on my side, but a question and a comment:

(q) it seems like we "just" moved the version indirection from feature flags to one layer up, but no big change in the Cargo.toml:

So we replace cargo build --features="${VERSION_ID}" with cargo build --bin=${VERSION_ID}

That might be something that comes up again, how is this improving how we track dependencies in the Cargo.lock?

I think in practice, it doesn't make much a difference whether we use features or not. What is important is splitting the binaries per version to allow fine granular control over the code per version. As long as we track this out of tree of rust-libp2p, I think this will be necessary.

With split binaries, it doesn't make much sense to have optional features any more because they don't conflict anyway.

(c) We're dropping dependency caching, i.e. "do not download and rebuild all my dependencies when I change one line in my main.rs".

I added this to make the experience of iterating on the test code more bearable. We might want to add back that feature eventually (through dummy mains, cargo-chef, or something else).

Yeah I've been there. I am okay with it being added back. I stripped it to simplify things as much as possible during debugging.

If somehow possible, I'd like to avoid containers for local iteration. Can testground run local binaries too?

Got it; thanks for the clarification,

If somehow possible, I'd like to avoid containers for local iteration. Can testground run local binaries too?

Yes, it doesn't do network isolation and simulation, so you might not be able to use this as the main workflow to develop & fix tests.

What is important is splitting the binaries per version to allow fine granular control over the code per version.

One last comment on this, if that's the point of this PR, is it possible we're trading "simple" for "easy" and we'll pay in the long term?

We're replacing "I have one main, with many shims for backward compatibility" with "I have many mains re-implementing a shared API".

The go version has been around for longer, so we have more examples of this:

One shared main

Shim some parts of the library (build flags in the header are ~= rust feature flags)

Note how surgical we can be when we support new versions, upgrading from v0.20 to v0.21 is "just" adding a flag because there was no API-breaking change.

With the new approach, it seems like we have 7 main files, ~600 lines, and only ~40 lines that are different, LOC is not a perfect metric, but it seems to indicate we're trading O(log(versions)) code complexity for O(versions).

One last comment on this, if that's the point of this PR, is it possible we're trading "simple" for "easy" and we'll pay in the long term?

I see your point. Though unfortunately I don't see how we can support future breaking changes in rust-libp2p with the current "simple" approach. In other words, I don't see an alternative to the "easy" approach going forward.

All that said, I think it is relatively easy to iterate on this part of the code base. In case we come up with a better design, let's refactor ping/rust.

(c) We're dropping dependency caching, i.e. "do not download and rebuild all my dependencies when I change one line in my main.rs".
I added this to make the experience of iterating on the test code more bearable. We might want to add back that feature eventually (through dummy mains, cargo-chef, or something else).

Yeah I've been there. I am okay with it being added back. I stripped it to simplify things as much as possible during debugging.

@thomaseizinger would you mind adding it back in? I suggest not blocking this pull request, but instead do so in a follow-up pull request.

With the new approach, it seems like we have 7 main files, ~600 lines, and only ~40 lines that are different, LOC is not a perfect metric, but it seems to indicate we're trading O(log(versions)) code complexity for O(versions).

What I'd like to mention here is that from my PoV, all old versions shouldn't really need maintaining so it is code that just sits there but doesn't have to be touched.

I know it looks scary but honestly, the only problem I can see is breaking changes in the Rust client of testground. Nothing else should warrant touching those old test binaries and even that we could work around by depending on the library multiple times, same as we do for libp2p itself.

Yeah I've been there. I am okay with it being added back. I stripped it to simplify things as much as possible during debugging.

@thomaseizinger would you mind adding it back in? I suggest not blocking this pull request, but instead do so in a follow-up pull request.

I'll have a look. Right now, I am just happy to be unblocked on my PR :)

jxs

LGTM Thomas 💪

jxs · 2022-11-17T16:20:02Z

ping/_compositions/rust.toml


 [[groups]]
 Id = "v0.47.0"
-CargoFeatures = 'libp2pv0470'
+BinaryName = 'testplan_0470'

 [[groups]]
 Id = "v0.46.0"


should here also be the 46.1?

Not sure, this is take verbatim from current master.

jxs · 2022-11-17T16:20:07Z

ping/_compositions/rust.toml


 [[groups]]
 Id = "v0.45.1"
-CargoFeatures = 'libp2pv0450'
+BinaryName = 'testplan_0450'


Suggested change

BinaryName = 'testplan_0450'

BinaryName = 'testplan_0451'

Can you send a follow-up if you care much? :)

I'd prefer to merge this asap to unblock libp2p/rust-libp2p#2972.

jxs · 2022-11-17T16:36:37Z

ping/rust/src/bin/testplan_0490.rs

+        development_transport(local_key).await?,
+        ping::Behaviour::new(
+            #[allow(deprecated)]
+            // TODO: Fixing this deprecation requires https://github.com/libp2p/rust-libp2p/pull/3055.


we are not using the derive macro here, do we need #[allow(deprecated)] ?

It is still a deprecated function!

mxinden · 2022-11-18T11:47:49Z

If I am not missing anything, we seem to have consensus that this is a step forward. Thanks for the work here @thomaseizinger. Thanks for the input @laurentsenta and @jxs.

Follow-ups to this pull request:

@jxs would you mind proposing a pull request that bumps to rust-libp2p v0.45.1 and v0.46.1?
@thomaseizinger could you please provide a patch re-introducing the Docker caching logic? See ping/rust: Refactor into multiple binaries #72 (comment).

thomaseizinger · 2022-11-19T08:09:14Z

Any sed experts here?

Something doesn't seem to quite work: https://github.com/libp2p/rust-libp2p/actions/runs/3499746591/jobs/5861640925

thomaseizinger added 3 commits November 8, 2022 16:33

Refactor into multiple rust binaries

aa08d37

Merge branch 'master' into 71-split-rust-crate

0e8d9a4

Fix some errors

6d1b820

mxinden reviewed Nov 8, 2022

View reviewed changes

This was referenced Nov 9, 2022

swarm-derive: Add prelude configuration option to NetworkBehaviour macro libp2p/rust-libp2p#3055

Merged

Run cargo update before build #68

Closed

Update master libp2p to make use of latest derive features

2056ae8

thomaseizinger mentioned this pull request Nov 14, 2022

Allow plans to point to an existing docker image testground/testground#1522

Open

mxinden approved these changes Nov 14, 2022

View reviewed changes

mxinden reviewed Nov 14, 2022

View reviewed changes

ping/_compositions/go-rust-interop-latest.toml Outdated Show resolved Hide resolved

mxinden changed the title ~~Refactor into multiple rust binaries~~ ping/rust: Refactor into multiple binaries Nov 14, 2022

thomaseizinger added 8 commits November 15, 2022 17:42

Explicitly specify git revision in manifest file

62460ea

Cargo updates dependency automatically if the manifest file changes

7d3decb

Make Dockerfile more maintenance friendly

a8889e1

Remove stale UpdateCmd

42bfd42

Fix and simplify seds

e69c165

Escape GIT_TARGET properly

f1cffef

Restore original Dockerfile from local testing

2c17e00

Add comment about build cache

16aff9d

thomaseizinger requested a review from mxinden November 15, 2022 08:07

thomaseizinger added 2 commits November 15, 2022 20:00

Put if on one line

b6d6549

Update README

69c7d14

mxinden reviewed Nov 15, 2022

View reviewed changes

Use alternative sed delimiter

6ac74b0

thomaseizinger added 8 commits November 16, 2022 09:53

Actually expand variables 🤦

40cfcac

Attempt to fix entrypoint

c95c03b

Try not quoting vars

5ede777

More Dockerfile debugging

8a739d3

More Dockerfile debugging

a632dde

More Dockerfile debugging

f53398c

More Dockerfile debugging

fff11a9

More Dockerfile debugging

7d2b5ab

thomaseizinger commented Nov 16, 2022

View reviewed changes

ping/rust/Dockerfile Outdated Show resolved Hide resolved

ping/rust/Dockerfile Outdated Show resolved Hide resolved

Apply suggestions from code review

ddc0d72

mxinden reviewed Nov 16, 2022

View reviewed changes

ping/rust/Dockerfile Outdated Show resolved Hide resolved

ping/rust/Dockerfile: Fix typo usrl usr

c5710e1

mxinden approved these changes Nov 16, 2022

View reviewed changes

ping/rust/Dockerfile Outdated Show resolved Hide resolved

thomaseizinger commented Nov 16, 2022

View reviewed changes

ping/rust/Dockerfile Outdated Show resolved Hide resolved

thomaseizinger added 3 commits November 17, 2022 09:36

Update ping/rust/Dockerfile

6bc971d

Move in previous container

e81e928

Move comment out the way

a4472fb

Copy binary directly into correct location

e50da9b

mxinden reviewed Nov 17, 2022

View reviewed changes

jxs approved these changes Nov 17, 2022

View reviewed changes

mxinden merged commit abce1ee into libp2p:master Nov 18, 2022

mxinden mentioned this pull request Nov 18, 2022

feat: Better error reporting when features are disabled libp2p/rust-libp2p#2972

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ping/rust: Refactor into multiple binaries #72

ping/rust: Refactor into multiple binaries #72

thomaseizinger commented Nov 8, 2022 •

edited

mxinden left a comment

mxinden left a comment

thomaseizinger commented Nov 14, 2022

thomaseizinger commented Nov 15, 2022

thomaseizinger commented Nov 15, 2022

thomaseizinger commented Nov 15, 2022

mxinden Nov 15, 2022

thomaseizinger Nov 15, 2022

mxinden commented Nov 15, 2022

thomaseizinger commented Nov 15, 2022

thomaseizinger commented Nov 16, 2022

thomaseizinger commented Nov 17, 2022

thomaseizinger commented Nov 17, 2022

mxinden Nov 17, 2022

laurentsenta Nov 17, 2022 •

edited

thomaseizinger Nov 17, 2022

laurentsenta Nov 18, 2022 •

edited

mxinden Nov 18, 2022

thomaseizinger Nov 18, 2022

thomaseizinger Nov 18, 2022

jxs left a comment

jxs Nov 17, 2022

thomaseizinger Nov 18, 2022

jxs Nov 17, 2022

thomaseizinger Nov 18, 2022

jxs Nov 17, 2022

thomaseizinger Nov 18, 2022

mxinden commented Nov 18, 2022

thomaseizinger commented Nov 19, 2022

		@@ -4,43 +4,23 @@ WORKDIR /usr/src/testplan
		RUN apt-get update && apt-get install -y cmake protobuf-compiler

ping/rust: Refactor into multiple binaries #72

ping/rust: Refactor into multiple binaries #72

Conversation

thomaseizinger commented Nov 8, 2022 • edited

mxinden left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 14, 2022

thomaseizinger commented Nov 15, 2022

thomaseizinger commented Nov 15, 2022

thomaseizinger commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Nov 15, 2022

thomaseizinger commented Nov 15, 2022

thomaseizinger commented Nov 16, 2022

thomaseizinger commented Nov 17, 2022

thomaseizinger commented Nov 17, 2022

Choose a reason for hiding this comment

laurentsenta Nov 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsenta Nov 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Nov 18, 2022

thomaseizinger commented Nov 19, 2022

thomaseizinger commented Nov 8, 2022 •

edited

laurentsenta Nov 17, 2022 •

edited

laurentsenta Nov 18, 2022 •

edited