-
Notifications
You must be signed in to change notification settings - Fork 598
feat: Noir sync rework #12574
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
feat: Noir sync rework #12574
Changes from all commits
3b3f8ca
f9ee1b9
920780a
1e44c52
abfd9c2
c6bedad
cddeec9
6e1424d
eea3c1f
25554a8
dd530f0
1c57904
1cadcba
d38dc05
9f7f83f
c2848b3
8bacdc3
e43b4a6
2b8836a
d849100
29c2c01
21e4c30
8859664
6c31aab
f87a15c
c2950be
ddaa3fb
bebe7a5
634acad
910fe7d
2a95481
db46e66
23790f7
6d81e4d
8ea110f
bd376b5
585a1fb
b11f823
2630f62
2eae01b
dab5208
cc14c5b
4991d55
36b534b
7107a61
e10525e
5da35e5
f85215d
4682ce5
c032119
55b9d6e
7cbeba0
ee75c71
e37c85c
ecb8a37
f19d89f
80f083a
698e5c0
fdd23a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,7 @@ function check_toolchains { | |
| exit 1 | ||
| fi | ||
| # Check rustup installed. | ||
| local rust_version=$(yq '.toolchain.channel' ./noir/noir-repo/rust-toolchain.toml) | ||
| local rust_version=$(yq '.toolchain.channel' ./avm-transpiler/rust-toolchain.toml) | ||
| if ! command -v rustup > /dev/null; then | ||
| encourage_dev_container | ||
| echo "Rustup not installed." | ||
|
|
@@ -114,6 +114,8 @@ function install_hooks { | |
| echo "./noir-projects/precommit.sh" >>$hooks_dir/pre-commit | ||
| echo "./yarn-project/constants/precommit.sh" >>$hooks_dir/pre-commit | ||
| chmod +x $hooks_dir/pre-commit | ||
| echo "(cd noir && ./postcheckout.sh $@)" >$hooks_dir/post-checkout | ||
| chmod +x $hooks_dir/post-checkout | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should do this right after checking out submodules? As this is basically "getting a submodule" our own way.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't actually check out the The other difference I think is that if I put it after the update of submodules, it would print this message whenever the developer runs a build and there are commits in the |
||
| } | ||
|
|
||
| function test_cmds { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| **/package.tgz | ||
| packages | ||
| noir-repo |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,40 @@ | ||
| # Aztec's Build of Noir | ||
|
|
||
| We subrepo noir into the folder `noir-repo`. | ||
| We subrepo [noir](https://github.com/noir-lang/noir) into the folder `noir-repo` during the build. | ||
| This folder contains dockerfiles and scripts for performing our custom build of noir for the monorepo. | ||
|
|
||
| # Syncing with the main Noir repository | ||
|
|
||
| In order to keep aztec-packages in step with the main Noir repository we need to periodically sync between them. | ||
|
|
||
| Syncing from aztec-packages into noir currently attempts to revert any changes in Noir since the last sync so it's recommended to always sync from Noir first to ensure that aztec-packages is up-to-date. | ||
| Ideally there should be a one-way sync from Noir to aztec-packages, but occasionally, when the `bb` interface changes, | ||
| or if some integration test fails and the Noir bug is fixed locally, changes have to go both ways. | ||
|
|
||
| ## Syncing from Noir to aztec-packages. | ||
| ## Syncing from Noir to aztec-packages | ||
|
|
||
| To start the sync run [this action](https://github.com/AztecProtocol/aztec-packages/actions/workflows/pull-noir.yml) manually (click the "Run Workflow" button in the top right). aztec-bot will then open a new PR which does the initial sync, this will have merge conflicts with master which will need to be resolved. | ||
| During the build the Noir repository is cloned or updated according to the contents of the [noir-repo-ref](./noir-repo-ref) | ||
| file, which can be a tag, branch name or commit hash. The value can be overriden using the `NOIR_REPO_REF` environment variable, | ||
| for example to run the integration tests in aztec-packages against a yet-to-be-released branch of Noir. | ||
|
|
||
| ## Syncing from aztec-packages to Noir. | ||
| If the ref contains a branch, it's pulled with each build triggered by `bootstrap.sh`, but for repeatable builds it should | ||
| point at a tag instead or commit instead, which would be updated with a regular PR opened in aztec-packages, so we can run | ||
| the test suite before changes take effect. | ||
|
|
||
| To start the sync run [this action](https://github.com/AztecProtocol/aztec-packages/actions/workflows/pull-noir.yml) manually (click the "Run Workflow" button in the top right). aztec-bot will then open a new PR which updates the reference. This will might merge conflicts with master which will need to be resolved. | ||
|
|
||
| ## Syncing from aztec-packages to Noir | ||
|
|
||
| When syncing from aztec-packages to Noir it's important to check that the latest release of `bb` uses the same ACIR serialization format as the current master commit. This is because Noir uses a released version of barretenberg rather than being developed in sync with it, it's then not possible to sync if there's been serialization changes since the latest release. | ||
|
|
||
| To start the sync run [this action](https://github.com/AztecProtocol/aztec-packages/actions/workflows/mirror-noir-subrepo.yml) manually (click the "Run Workflow" button in the top right). aztec-bot will then open a new PR in the `noir-lang/noir` repository which does the initial sync, this will have merge conflicts with master which will need to be resolved. | ||
| When we make changes in `noir-repo` and commit them, we can check out a branch and push them back to Noir, where a PR can be opened to merge them back | ||
| into an appropriate branch (could be `master` or some kind of integration branch). It is important to exclude the [fixup](./scripts/sync-in-fixup.sh) that the local checkout performs from the PR by running the [fixdown](./scripts/sync-out-fixup.sh) script. | ||
|
|
||
| Syncing can be postponed by creating a few commits in `noir-repo`, but instead of opening a PR against Noir, creating a [git patch](https://git-scm.com/docs/git-format-patch) instead using, which is committed to aztec-packages and is applied to any subsequent checkout. A patch file can be made using the following command: | ||
|
|
||
| ```shell | ||
| ./bootstrap.sh make-patch | ||
| ``` | ||
|
|
||
| After this `./noir-repo.patch` should have the changes committed on top of the latest checkout, and if we commit this file to `aztec-packages` then it is automatically applied by any subsequent checkouts of `noir-repo`. | ||
|
|
||
| To start an automated sync run [this action](https://github.com/AztecProtocol/aztec-packages/actions/workflows/mirror-noir-subrepo.yml) manually (click the "Run Workflow" button in the top right). aztec-bot will then open a new PR in the `noir-lang/noir` repository which does the initial sync, this will have merge conflicts with master which will need to be resolved. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,18 @@ | ||
| #!/usr/bin/env bash | ||
| source $(git rev-parse --show-toplevel)/ci3/source_bootstrap | ||
|
|
||
| set -eou pipefail | ||
|
|
||
| cmd=${1:-} | ||
| [ -n "$cmd" ] && shift | ||
|
|
||
| # Update the noir-repo before we hash its content, unless the command is exempt. | ||
| no_update=(clean make-patch bump-noir-repo-ref) | ||
| if [[ -z "$cmd" || ! ${no_update[*]} =~ "$cmd" ]]; then | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just hash the ref and the patch file? (tho we should still disable the cache if there's any local changes to noir repo, hmm)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't want to fundamentally change what the hashes work with: have we compiled and cached this code or not, including pending changes and local commits that may not be in the hash file. |
||
| scripts/sync.sh init | ||
| scripts/sync.sh update | ||
| fi | ||
|
|
||
| export hash=$(cache_content_hash .rebuild_patterns) | ||
| export test_hash=$(cache_content_hash .rebuild_patterns .rebuild_patterns_tests) | ||
|
|
||
|
|
@@ -183,9 +192,27 @@ function release_commit { | |
| release_packages next "$CURRENT_VERSION-commit.$COMMIT_HASH" | ||
| } | ||
|
|
||
| # Bump the Noir repo reference on a given branch to a given ref. | ||
| # The branch might already exist, e.g. this could be a daily job bumping the version to the | ||
| # latest nightly, and we might have to deal with updating the patch file because the latest | ||
| # Noir code conflicts with the contents of the patch, or we're debugging some integration | ||
| # test failure on CI. In that case just push another commit to the branch to bump the version | ||
| # further without losing any other commit on the branch. | ||
| function bump_noir_repo_ref { | ||
| branch=$1 | ||
| ref=$2 | ||
| git fetch --depth 1 origin $branch || true | ||
| git checkout --track origin/$branch || git checkout $branch || git checkout -b $branch | ||
| scripts/sync.sh write-noir-repo-ref $ref | ||
| git add . | ||
| git commit -m "chore: Update noir-repo-ref to $ref" || true | ||
| do_or_dryrun git push --set-upstream origin $branch | ||
| } | ||
|
|
||
| case "$cmd" in | ||
| "clean") | ||
| git clean -fdx | ||
| # Double `f` needed to delete the nested git repository. | ||
| git clean -ffdx | ||
| ;; | ||
| "ci") | ||
| build | ||
|
|
@@ -203,6 +230,12 @@ case "$cmd" in | |
| "hash-test") | ||
| echo $test_hash | ||
| ;; | ||
| "make-patch") | ||
| scripts/sync.sh make-patch | ||
| ;; | ||
| "bump-noir-repo-ref") | ||
| bump_noir_repo_ref $@ | ||
| ;; | ||
| *) | ||
| echo "Unknown command: $cmd" | ||
| exit 1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| nightly-2025-03-07 |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know if you guys still want a workflow to make PRs into noir with the patch, but otherwise we can delete the other side of the sync noir workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought opening a PR could involve some mechanical steps, such as rebasing on the remote branch without the fixup and patch marker commit, which could benefit from automation, even if it's just manually triggered, but we should definitely disable any automation for now. I thought we can return to this question once we actually have a situation and understand better what's a nice way to do it. @TomAFrench ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm fine with doing this manually in the short term and automating it later.