Add some Rust smoketesting CI for barretenberg_wrapper #14

phated · 2023-01-11T19:01:47Z

This adds some CI for the barretenberg_wrapper crate. I wanted to get this added so we are more comfortable with changes such as my #9 PR and #8 (which we'll want to rebase on this). I believe ARM support is on the GitHub Action roadmap, so we'll be able to expand this matrix in the future.

I also needed to fix some rust formatting and clippy stuff in separate commits.

I'll also have some comments inline.

Depends upon #13

phated · 2023-01-11T19:04:57Z

.github/workflows/rust.yml

+      # TODO: Enable when tests work
+      # - name: Run cargo test
+      #   working-directory: "./barretenberg_wrapper"
+      #   run: |
+      #     cargo +${{ matrix.toolchain }} test


On linux, running cargo test fails with a ton of linker errors. I'm not sure why.

On Mac, the tests pedersen tests fail with (what looks like) an ordering problem.

I'd love some help getting the tests working in this project.

I've updated the tests to match aztec_backend as per @vezenovm's suggestions and re-enabled the test step. I'm guessing we'll still have issues with linking on Linux though.

The linux tests were failing because rust picks up gcc as the linker on ubuntu. I've added a config file to override that as per rust-lang/rust#71515 (comment)

Also, it seems by specifying this inside of barretenberg_wrapper, it'll apply when used as a dependency as per the Hierarchical Structure documentation: https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure

barretenberg/src/aztec/crypto/pedersen/sidon_set/sidon_set.hpp

barretenberg/src/aztec/crypto/schnorr/multisig.hpp

barretenberg_wrapper/src/composer.rs

barretenberg_wrapper/src/pedersen.rs

phated · 2023-01-11T21:37:46Z

barretenberg_wrapper/.cargo/config.toml

@@ -0,0 +1,2 @@
+[target.x86_64-unknown-linux-gnu]


I believe we'll want to add something similar for aarch64-unknown-linux-gnu in PR #8

…-linux-gnu

TomAFrench · 2023-01-11T22:11:38Z

process didn't exit successfully: /home/runner/work/aztec-connect/aztec-connect/barretenberg_wrapper/target/debug/deps/barretenberg_wrapper-c6045f1b11456fbf (signal: 4, SIGILL: illegal instruction)

This looks like the same error message I'm getting when trying to build barretenberg locally. There's something in the test files which is borked (everything builds fine if you delete all the tests).

phated · 2023-01-11T22:13:18Z

@TomAFrench It looks flakey, as they pass most of the time. I updated a couple tests that weren't updated but maybe there are others that need to be updated. I'll ask @vezenovm

TomAFrench · 2023-01-11T22:17:47Z

.github/workflows/rust.yml

@@ -0,0 +1,93 @@
+name: Rust
+
+on: [push, pull_request]


Suggested change

on: [push, pull_request]

on:

push:

branches: 'kw/noir-dsl'

pull_request:

branches: '*'

This currently runs CI twice on PRs.

Running the PR twice is actually good because a push runs with the branch as it's own base and pull_request runs with the upstream as the base.

I'm not sure I follow on this. When you say "with the upstream as the base" do you mean "as if the PR were rebased on top of the branch being merged into"? I haven't heard anything about push and pull_request treating the same commit differently.

As per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

Thus, the checkout action running on something triggered by pull_request will checkout a merge between the upstream and the PR and test it. And push just checks out to the commit that is pushed.

Hmm, interesting. I didn't know that GH did that temporary merge for pull_request.

Personally I generally try to avoid merging branches into a base branch which is ahead of it (e.g. through first merging back into the PR branch) to check this explicitly rather than relying on GH.

TomAFrench · 2023-01-11T22:19:54Z

It looks flakey, as they pass most of the time.

Unfortunately it's 100% of the time for me which prevents me from building anything noir-related.

phated · 2023-01-11T22:22:39Z

Unfortunately it's 100% of the time for me which prevents me from building anything noir-related.

I'm confused, why are the tests being built if you are building Noir itself? This issue is only showing up when the tests are run in this repository.

There was a separate Illegal instruction failure within the googletest stuff, but that should be disabled since 2630271 - Please note that always_configure is set to false so you actually need to cargo clean to ensure CMake configuration is being re-done.

jfecher · 2023-01-11T22:23:28Z

I can confirm that I also get the Illegal Instruction error in the google tests 100% of the time when building barretenberg (on my other machine - linux x86_64).

TomAFrench · 2023-01-11T22:35:58Z

Aha, I've just been running bootstrap.sh in the barrentenberg directory directly as it seemed sensible to get that building and then work my way up. This means that we don't end up turning off the TESTING flag like we do in build.rs.

I've added that manually to ./bootstrap.sh and I can build properly now.

phated · 2023-01-11T22:38:03Z

Oh, I wouldn't trust the bootstrap.sh file. There's a build.rs file in barretenberg_wrapper that builds barretenberg and configures rust to use it.

I suppose we can add the -DTESTING=OFF flag though.

TomAFrench · 2023-01-11T22:51:38Z

I'm confused, why are the tests being built if you are building Noir itself? This issue is only showing up when the tests are run in this repository.

There was a separate Illegal instruction failure within the googletest stuff, but that should be disabled since 2630271

Yeah, I think the ./bootstrap.sh stuff is a bit of a red herring now.

When building nargo I get the errors fixed in #13 and 2630271. These are fixed now and nargo just needs to have dependencies updated. Not knowing this, I came here to try and hunt down the issue and once Barretenberg starts throwing the same error as nargo did, it's natural to try and troubleshoot there.

phated · 2023-01-11T23:12:04Z

nargo just needs to have dependencies updated

Yep! I'm working on getting everything updated so nargo builds. I just started on Monday and nargo wouldn't build on my Mac with everything fresh. So, of course, I've been on a mission to make sure it works! Just waiting on a few PRs across various repos.

TomAFrench · 2023-01-11T23:14:26Z

Thanks, I apprecate it! I just tested with your PR on aztec_backend and it allowed me to build nargo again after quite some time.

jfecher

LGTM 👍

* Cargo format * Clippy fixes * Update tests to match tests in aztec_backend * Add cargo config to set clang as linker when target is x86_64-unknown-linux-gnu

phated requested review from kevaundray and kobyhallx January 11, 2023 19:01

phated commented Jan 11, 2023

View reviewed changes

barretenberg/src/aztec/crypto/pedersen/sidon_set/sidon_set.hpp Outdated Show resolved Hide resolved

phated commented Jan 11, 2023

View reviewed changes

barretenberg/src/aztec/crypto/schnorr/multisig.hpp Outdated Show resolved Hide resolved

phated commented Jan 11, 2023

View reviewed changes

barretenberg_wrapper/src/composer.rs Show resolved Hide resolved

phated commented Jan 11, 2023

View reviewed changes

barretenberg_wrapper/src/pedersen.rs Show resolved Hide resolved

phated commented Jan 11, 2023

View reviewed changes

barretenberg_wrapper/src/pedersen.rs Show resolved Hide resolved

phated force-pushed the bb/rust-smoketests branch from 1159376 to b1959d3 Compare January 11, 2023 20:08

phated requested a review from jfecher January 11, 2023 20:08

phated added 4 commits January 11, 2023 13:45

Add some Rust smoketesting CI for barretenberg_wrapper

68b221b

Cargo format

261e33d

Clippy fixes

1fe4d1a

Update tests to match tests in aztec_backend

f4936a5

phated force-pushed the bb/rust-smoketests branch 2 times, most recently from 10ab90b to 4070246 Compare January 11, 2023 21:35

phated commented Jan 11, 2023

View reviewed changes

Add cargo config to set clang as linker when target is x86_64-unknown…

08a95c5

…-linux-gnu

phated force-pushed the bb/rust-smoketests branch from 4070246 to 08a95c5 Compare January 11, 2023 22:00

TomAFrench reviewed Jan 11, 2023

View reviewed changes

phated mentioned this pull request Jan 11, 2023

Disable TESTING option inside barretenberg bootstrap #15

Merged

phated requested a review from vezenovm January 11, 2023 23:24

Merge branch 'kw/noir-dsl' into bb/rust-smoketests

ac06c1d

jfecher approved these changes Jan 12, 2023

View reviewed changes

vezenovm approved these changes Jan 12, 2023

View reviewed changes

phated merged commit ae1d7cd into kw/noir-dsl Jan 12, 2023

phated deleted the bb/rust-smoketests branch January 12, 2023 15:08

phated mentioned this pull request Jan 12, 2023

Update barretenberg_wrapper to use branch noir-lang/acvm-backend-barretenberg#37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some Rust smoketesting CI for barretenberg_wrapper #14

Add some Rust smoketesting CI for barretenberg_wrapper #14

phated commented Jan 11, 2023

phated Jan 11, 2023

phated Jan 11, 2023

phated Jan 11, 2023

phated Jan 11, 2023 •

edited

Loading

phated Jan 11, 2023 •

edited

Loading

TomAFrench commented Jan 11, 2023

phated commented Jan 11, 2023

TomAFrench Jan 11, 2023 •

edited

Loading

phated Jan 11, 2023 •

edited

Loading

TomAFrench Jan 11, 2023

phated Jan 11, 2023

TomAFrench Jan 12, 2023

TomAFrench commented Jan 11, 2023

phated commented Jan 11, 2023

jfecher commented Jan 11, 2023 •

edited

Loading

TomAFrench commented Jan 11, 2023 •

edited

Loading

phated commented Jan 11, 2023 •

edited

Loading

TomAFrench commented Jan 11, 2023 •

edited

Loading

phated commented Jan 11, 2023

TomAFrench commented Jan 11, 2023

jfecher left a comment

-on: [push, pull_request]
+on:
+  push:
+    branches: 'kw/noir-dsl'
+  pull_request:
+    branches: '*'

Add some Rust smoketesting CI for barretenberg_wrapper #14

Add some Rust smoketesting CI for barretenberg_wrapper #14

Conversation

phated commented Jan 11, 2023

phated Jan 11, 2023

Choose a reason for hiding this comment

phated Jan 11, 2023

Choose a reason for hiding this comment

phated Jan 11, 2023

Choose a reason for hiding this comment

phated Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

phated Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

TomAFrench commented Jan 11, 2023

phated commented Jan 11, 2023

TomAFrench Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

phated Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

TomAFrench Jan 11, 2023

Choose a reason for hiding this comment

phated Jan 11, 2023

Choose a reason for hiding this comment

TomAFrench Jan 12, 2023

Choose a reason for hiding this comment

TomAFrench commented Jan 11, 2023

phated commented Jan 11, 2023

jfecher commented Jan 11, 2023 • edited Loading

TomAFrench commented Jan 11, 2023 • edited Loading

phated commented Jan 11, 2023 • edited Loading

TomAFrench commented Jan 11, 2023 • edited Loading

phated commented Jan 11, 2023

TomAFrench commented Jan 11, 2023

jfecher left a comment

Choose a reason for hiding this comment

phated Jan 11, 2023 •

edited

Loading

phated Jan 11, 2023 •

edited

Loading

TomAFrench Jan 11, 2023 •

edited

Loading

phated Jan 11, 2023 •

edited

Loading

jfecher commented Jan 11, 2023 •

edited

Loading

TomAFrench commented Jan 11, 2023 •

edited

Loading

phated commented Jan 11, 2023 •

edited

Loading

TomAFrench commented Jan 11, 2023 •

edited

Loading