ci: Add a step running complement crypto

[kegsay]

WIP to get CI to work for now.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.97%. Comparing base (3517334) to head (ab0abca).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3400      +/-   ##
==========================================
- Coverage   82.98%   82.97%   -0.01%     
==========================================
  Files         246      246              
  Lines       25022    25022              
==========================================
- Hits        20764    20763       -1     
- Misses       4258     4259       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bnjbvr]

I'm worried that we don't use a fixed/pinned version here, because that means that now our CI may fail because of external causes only (if complement-crypto fails because a new test was pushed to main and it failed, then our CI fails too). On the other hand, using a pinned version would mean bumping here. What are your thoughts on this?

[kegsay]

It's fully intended that as new tests are added to complement crypto that they are tested in rust SDK CI. Having to make a new PR to include these new tests adds friction which will decrease the likelihood of these tests being regularly run.

I share your concern though, which is why the complement crypto repo also runs the same CI against rust-sdk main so if a new test fails in rust SDK, we'll know about it when the complement crypto PR is made, rather than waiting for things to break in rust SDK CI.

[Hywan]

Is it a problem to release Complement often, like very frequently?

[kegsay]

Complement Crypto doesn't have any versioned releases yet. It's not clear what the semvers would even be referencing, given there isn't a public API which can have breaking changes. Rust SDK isn't the only user here, so any releases would really need to make sense for both rather than preferentially rust SDK.

[bnjbvr]

Sounds good, we can try it out like this and advise later if needs be. Thanks for the reasoning.

[bnjbvr]

Nice! Getting more testing is good, and it's good that it doesn't slow down the entire testing pipeline.

Before merging this, I'd like two more things please:

• a rough note somewhere (README? Contribute.md?) explaining how to reproduce the complement crypto setup locally, so that one can reproduce Complement CI failures easily. Or at the very least, a link to the right explanation; it seems that https://github.com/matrix-org/complement-crypto/?tab=readme-ov-file#how-do-i-run-it doesn't contain such a simplified / streamlined way to do this.
• if we changed the Rust FFI layer in a breaking way, (which we do all the time without really thinking about external users, since it's often coordinated internally with Element app devs), could that break the Complement testing suite, as it's making use of functions exported by the FFI layer? If so, how do we "break" the dependency loop in such a case? Complement would need to be updated with the latest version of the SDK, but the API change in the SDK couldn't be merged because the Complement step would fail. We need to figure this out ahead of time, and have an appropriate process when that happens, otherwise we'll really soon run into such an issue.

[kegsay]

@bnjbvr please can you check if the instructions on https://github.com/matrix-org/complement-crypto/tree/main work for you / if they are sufficiently clear.

As for the dependency loop, what synapse x complement do is:

• Make a branch foo on synapse for new code.
• Make a branch foo on complement for new code.
• When the synapse PR is made, CI looks for a complement branch with exactly the same name and tests using that branch, else falls back to main.
• When the complement PR is made, CI looks for a synapse branch with exactly the same name and tests using that branch, else falls back to main.

This allows breaking changes to be made and tested in PR form, before landing both in their respective repos. Would something similar to this be acceptable?

[bnjbvr]

Thanks, it makes sense. I will try the new documentation now.

In the spirit of more asynchronicity, one question though:

When the synapse PR is made, CI looks for a complement branch with exactly the same name and tests using that branch, else falls back to main.

This would require extra work in our CI, right? Are you intending to do this now, or is someone assigned to do it later? If the latter, and the dependency cycle situation happens before it's implemented, would it be fine to disable the Complement Crypto CI step in the meanwhile?

[bnjbvr]

I managed to run Complement Crypto locally \o/ Thanks @kegsay for the new instructions.

Some notes from the field. I'm not sure where to put them, as they're valid right now, for me, and might not be applicable to other folks:

• podman doesn't seem to automatically poll images, so a few ones needed to be polled manually before
  running the test:
  • synapse: podman pull ghcr.io/matrix-org/synapse-service:v1.94.0
  • postgres:
    • rg postgres to find the reference to the postgres image used by the test code (e.g. found
      "postgres:13-alpine" in internal/deploy/deploy.go)
    • podman pull docker.io/postgres:13-alpine
  • mitmproxy: podman pull docker.io/mitmproxy/mitmproxy:10.1.5
• start the podman service: systemctl --user start podman.service

and then run with:

COMPLEMENT_CRYPTO_TEST_CLIENT_MATRIX=rr \
      COMPLEMENT_BASE_IMAGE=ghcr.io/matrix-org/synapse-service:v1.94.0 \
      LD_LIBRARY_PATH=/home/work/code/matrix-rust-sdk/target/debug \
      LIBRARY_PATH=/home/work/code/matrix-rust-sdk/target/debug \
      GOTMPDIR=/home/work/safe/tmp/dir/ \
      DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock \
      BUILDAH_FORMAT=docker \
      COMPLEMENT_HOSTNAME_RUNNING_COMPLEMENT=host.containers.internal \
        go test -v -count=1 -tags=rust -timeout 15m ./tests

I'll do the formal approval once we have answers to #3400 (comment).

[kegsay]

This would require extra work in our CI, right? Are you intending to do this now, or is someone assigned to do it later? If the latter, and the dependency cycle situation happens before it's implemented, would it be fine to disable the Complement Crypto CI step in the meanwhile?

It would require changes to this PR to look at the right branch and fallback to main, rather than always using main which is what it does right now. I would be happy to block this PR behind this work to force it to be done, but it will be a few days until I come back to this.

[bnjbvr]

Cool, I'd rather block the PR so that we don't have to disable the Complement Crypto earlier (if needs be). Thanks!

[kegsay]

This is ready to go but is blocked by a failing test. The failing test is legit (it's a bug in the rust SDK) but it cannot be reliably reproduced yet, so I'm going to skip it until I have time to look into this some more. When that is done, I'll re-request a review.

[kegsay]

The flakey test has been skipped.

[bnjbvr]

Thanks!