Skip to content

Commit

Permalink
Merge branch 'master' into aztec-packages
Browse files Browse the repository at this point in the history
* master:
  fix: detect cycles in globals (#6859)
  chore(ci): Execution time report (#6827)
  chore(ci): Add non determinism check and fixes (#6847)
  chore(docs): updating the solidity contract how-to guide (#6804)
  fix: double alias in path (#6855)
  feat: configurable external check failures (#6810)
  chore: move constant creation out of loop (#6836)
  fix: implement `as_field` and `from_field` in the interpreter (#6829)
  chore: Use Vec for callstacks (#6821)
  feat: replace `eval_global_as_array_length` with type/interpreter evaluation (#6469)
  chore: refactor `DataFlowGraph.insert_instruction_and_results` (#6823)
  chore(docs): updating noirjs tutorial for 1.0.0 (#6792)
  • Loading branch information
TomAFrench committed Dec 18, 2024
2 parents 3bda279 + 0d7642c commit fbff386
Show file tree
Hide file tree
Showing 77 changed files with 1,020 additions and 651 deletions.
20 changes: 20 additions & 0 deletions .github/critical_libraries_status/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Critical Libraries Status

This directory contains one `.failures.jsonl` file per external directory that is checked by CI.
CI will run the external repository tests and compare the test failures against those recorded
in these files. If there's a difference, CI will fail.

This allows us to mark some tests as expected to fail if we introduce breaking changes.
When tests are fixed on the external repository, CI will let us know that we need to remove
the `.failures.jsonl` failures on our side.

The format of the `.failures.jsonl` files is one JSON per line with a failure:

```json
{"suite":"one","name":"foo"}
```

If it's expected that an external repository doesn't compile (because a PR introduces breaking changes
to, say, the type system) you can remove the `.failures.jsonl` file for that repository and CI
will pass again. Once the repository compiles again, CI will let us know and require us to put
back the `.failures.jsonl` file.
4 changes: 4 additions & 0 deletions .github/critical_libraries_status/noir-lang/ec/.actual.jsonl
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{"event":"started","name":"one","test_count":1,"type":"suite"}
{"event":"started","name":"foo","suite":"one","type":"test"}
{"event":"failed","exec_time":0.05356625,"name":"foo","suite":"one","type":"test"}
{"event":"ok","failed":0,"ignored":0,"passed":1,"type":"suite"}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"suite":"one","name":"foo"}
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
38 changes: 38 additions & 0 deletions .github/scripts/check_test_results.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/bin/bash
set -eu

# Usage: ./check_test_results.sh <expected.json> <actual.jsonl>
# Compares the output of two test results of the same repository.
# If any of the files doesn't exist or is empty, the script will consider that the test suite
# couldn't be compiled.

function process_json_lines() {
cat $1 | jq -c 'select(.type == "test" and .event == "failed") | {suite: .suite, name: .name}' | jq -s -c 'sort_by(.suite, .name) | .[]' > $1.jq
}

if [ -f $1 ] && [ -f $2 ]; then
# Both files exist, let's compare them
$(process_json_lines $2)
if ! diff $1 $2.jq; then
echo "Error: test failures don't match expected failures"
echo "Lines prefixed with '>' are new test failures (you could add them to '$1')"
echo "Lines prefixed with '<' are tests that were expected to fail but passed (you could remove them from '$1')"
fi
elif [ -f $1 ]; then
# Only the expected file exists, which means the actual test couldn't be compiled.
echo "Error: external library tests couldn't be compiled."
echo "You could rename '$1' to '$1.does_not_compile' if it's expected that the external library can't be compiled."
exit -1
elif [ -f $2 ]; then
# Only the actual file exists, which means we are expecting the external library
# not to compile but it did.
echo "Error: expected external library not to compile, but it did."
echo "You could create '$1' with these contents:"
$(process_json_lines $2)
cat $2.jq
exit -1
else
# Both files don't exists, which means we are expecting the external library not
# to compile, and it didn't, so all is good.
exit 0
fi
118 changes: 81 additions & 37 deletions .github/workflows/reports.yml
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,8 @@ jobs:
retention-days: 3
overwrite: true

generate_compilation_report:
name: Compilation time
generate_compilation_and_execution_report:
name: Compilation and execution time
needs: [build-nargo]
runs-on: ubuntu-22.04
permissions:
Expand All @@ -253,34 +253,47 @@ jobs:
working-directory: ./test_programs
run: |
./compilation_report.sh
cat compilation_report.json
mv compilation_report.json ../compilation_report.json
- name: Generate Execution report
working-directory: ./test_programs
run: |
./execution_report.sh
mv execution_report.json ../execution_report.json
- name: Upload compilation report
uses: actions/upload-artifact@v4
with:
name: in_progress_compilation_report
path: compilation_report.json
retention-days: 3
overwrite: true

- name: Upload execution report
uses: actions/upload-artifact@v4
with:
name: in_progress_execution_report
path: execution_report.json
retention-days: 3
overwrite: true

external_repo_compilation_report:
external_repo_compilation_and_execution_report:
needs: [build-nargo]
runs-on: ubuntu-latest
timeout-minutes: 15
strategy:
fail-fast: false
matrix:
include:
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, is_library: true }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/parity-root }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public }

name: External repo compilation report - ${{ matrix.project.repo }}/${{ matrix.project.path }}
name: External repo compilation and execution reports - ${{ matrix.project.repo }}/${{ matrix.project.path }}
steps:
- name: Download nargo binary
uses: actions/download-artifact@v4
Expand All @@ -302,6 +315,13 @@ jobs:
sparse-checkout: |
test_programs/compilation_report.sh
sparse-checkout-cone-mode: false

- uses: actions/checkout@v4
with:
path: scripts
sparse-checkout: |
test_programs/execution_report.sh
sparse-checkout-cone-mode: false

- name: Checkout
uses: actions/checkout@v4
Expand All @@ -316,28 +336,53 @@ jobs:
mv /home/runner/work/noir/noir/scripts/test_programs/compilation_report.sh ./compilation_report.sh
chmod +x ./compilation_report.sh
./compilation_report.sh 1
- name: Generate execution report
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.is_library }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
./execution_report.sh 1
- name: Move compilation report
id: report
id: compilation_report
shell: bash
run: |
PACKAGE_NAME=${{ matrix.project.path }}
PACKAGE_NAME=$(basename $PACKAGE_NAME)
mv ./test-repo/${{ matrix.project.path }}/compilation_report.json ./compilation_report_$PACKAGE_NAME.json
echo "compilation_report_name=$PACKAGE_NAME" >> $GITHUB_OUTPUT
- name: Move execution report
id: execution_report
shell: bash
if: ${{ !matrix.project.is_library }}
run: |
PACKAGE_NAME=${{ matrix.project.path }}
PACKAGE_NAME=$(basename $PACKAGE_NAME)
mv ./test-repo/${{ matrix.project.path }}/execution_report.json ./execution_report_$PACKAGE_NAME.json
echo "execution_report_name=$PACKAGE_NAME" >> $GITHUB_OUTPUT
- name: Upload compilation report
uses: actions/upload-artifact@v4
with:
name: compilation_report_${{ steps.report.outputs.compilation_report_name }}
path: compilation_report_${{ steps.report.outputs.compilation_report_name }}.json
name: compilation_report_${{ steps.compilation_report.outputs.compilation_report_name }}
path: compilation_report_${{ steps.compilation_report.outputs.compilation_report_name }}.json
retention-days: 3
overwrite: true

- name: Upload execution report
uses: actions/upload-artifact@v4
with:
name: execution_report_${{ steps.execution_report.outputs.execution_report_name }}
path: execution_report_${{ steps.execution_report.outputs.execution_report_name }}.json
retention-days: 3
overwrite: true

upload_compilation_report:
name: Upload compilation report
needs: [generate_compilation_report, external_repo_compilation_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_report` fails
needs: [generate_compilation_and_execution_report, external_repo_compilation_and_execution_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_and_execution_report` fails
if: always()
runs-on: ubuntu-latest
permissions:
Expand Down Expand Up @@ -491,48 +536,47 @@ jobs:
header: memory
message: ${{ steps.memory_report.outputs.markdown }}

generate_compilation_report:
name: Compilation time
needs: [build-nargo]
runs-on: ubuntu-22.04
upload_execution_report:
name: Upload execution report
needs: [generate_compilation_and_execution_report, external_repo_compilation_and_execution_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_and_execution_report` fails
if: always()
runs-on: ubuntu-latest
permissions:
pull-requests: write

steps:
- uses: actions/checkout@v4

- name: Download nargo binary
- name: Download initial execution report
uses: actions/download-artifact@v4
with:
name: nargo
path: ./nargo
name: in_progress_execution_report

- name: Set nargo on PATH
run: |
nargo_binary="${{ github.workspace }}/nargo/nargo"
chmod +x $nargo_binary
echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
export PATH="$PATH:$(dirname $nargo_binary)"
nargo -V
- name: Download matrix execution reports
uses: actions/download-artifact@v4
with:
pattern: execution_report_*
path: ./reports

- name: Generate Compilation report
working-directory: ./test_programs
- name: Merge execution reports using jq
run: |
./compilation_report.sh
mv compilation_report.json ../compilation_report.json
mv ./.github/scripts/merge-bench-reports.sh merge-bench-reports.sh
./merge-bench-reports.sh execution_report
- name: Parse compilation report
id: compilation_report
uses: noir-lang/noir-bench-report@0d7464a8c39170523932d7846b6e6b458a294aea
- name: Parse execution report
id: execution_report
uses: noir-lang/noir-bench-report@0954121203ee55dcda5c7397b9c669c439a20922
with:
report: compilation_report.json
report: execution_report.json
header: |
# Compilation Report
memory_report: false
# Execution Report
execution_report: true

- name: Add memory report to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
header: compilation
message: ${{ steps.compilation_report.outputs.markdown }}
header: execution_time
message: ${{ steps.execution_report.outputs.markdown }}

17 changes: 13 additions & 4 deletions .github/workflows/test-js-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,6 @@ jobs:
external-repo-checks:
needs: [build-nargo, critical-library-list]
runs-on: ubuntu-22.04
# Only run when 'run-external-checks' label is present
if: contains(github.event.pull_request.labels.*.name, 'run-external-checks')
timeout-minutes: 30
strategy:
fail-fast: false
Expand All @@ -557,11 +555,17 @@ jobs:
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/parity-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/reset-kernel-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/types }
# Use 1 test threads for rollup-lib because each test requires a lot of memory, and multiple ones in parallel exceed the maximum memory limit.
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-lib, nargo_args: "--test-threads 1" }

name: Check external repo - ${{ matrix.project.repo }}/${{ matrix.project.path }}
steps:
- name: Checkout
uses: actions/checkout@v4
with:
path: noir-repo

- name: Checkout
uses: actions/checkout@v4
with:
Expand Down Expand Up @@ -592,9 +596,14 @@ jobs:
- name: Run nargo test
working-directory: ./test-repo/${{ matrix.project.path }}
run: nargo test -q --silence-warnings
run: |
out=$(nargo test --silence-warnings --skip-brillig-constraints-check --format json ${{ matrix.project.nargo_args }}) && echo "$out" > ${{ github.workspace }}/noir-repo/.github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl
env:
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true

- name: Compare test results
working-directory: ./noir-repo
run: .github/scripts/check_test_results.sh .github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.failures.jsonl .github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl

# This is a job which depends on all test jobs and reports the overall status.
# This allows us to add/remove test jobs without having to update the required workflows.
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ gates_report.json
gates_report_brillig.json
gates_report_brillig_execution.json
compilation_report.json
execution_report.json

# Github Actions scratch space
# This gives a location to download artifacts into the repository in CI without making git dirty.
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ pub struct CompileOptions {
/// A lower value keeps the original program if it was smaller, even if it has more jumps.
#[arg(long, hide = true, allow_hyphen_values = true)]
pub max_bytecode_increase_percent: Option<i32>,

/// Used internally to test for non-determinism in the compiler.
#[clap(long, hide = true)]
pub check_non_determinism: bool,
}

pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::Error> {
Expand Down
4 changes: 1 addition & 3 deletions compiler/noirc_evaluator/src/acir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2905,7 +2905,6 @@ mod test {
ssa::{
function_builder::FunctionBuilder,
ir::{
call_stack::CallStack,
function::FunctionId,
instruction::BinaryOp,
map::Id,
Expand All @@ -2932,8 +2931,7 @@ mod test {
builder.new_function("foo".into(), foo_id, inline_type);
}
// Set a call stack for testing whether `brillig_locations` in the `GeneratedAcir` was accurately set.
let mut stack = CallStack::unit(Location::dummy());
stack.push_back(Location::dummy());
let stack = vec![Location::dummy(), Location::dummy()];
let call_stack =
builder.current_function.dfg.call_stack_data.get_or_insert_locations(stack);
builder.set_call_stack(call_stack);
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub(crate) struct GeneratedBrillig<F> {
pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,
pub(crate) error_types: BTreeMap<ErrorSelector, ErrorType>,
pub(crate) name: String,
pub(crate) procedure_locations: HashMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
pub(crate) procedure_locations: BTreeMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
}

#[derive(Default, Debug, Clone)]
Expand Down Expand Up @@ -61,7 +61,7 @@ pub(crate) struct BrilligArtifact<F> {
/// This is created as artifacts are linked together and allows us to determine
/// which opcodes originate from reusable procedures.s
/// The range is inclusive for both start and end opcode locations.
pub(crate) procedure_locations: HashMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
pub(crate) procedure_locations: BTreeMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
}

/// A pointer to a location in the opcode.
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ impl RuntimeError {
RuntimeError::UnknownLoopBound { .. } => {
let primary_message = self.to_string();
let location =
self.call_stack().back().expect("Expected RuntimeError to have a location");
self.call_stack().last().expect("Expected RuntimeError to have a location");

Diagnostic::simple_error(
primary_message,
Expand All @@ -212,7 +212,7 @@ impl RuntimeError {
_ => {
let message = self.to_string();
let location =
self.call_stack().back().unwrap_or_else(|| panic!("Expected RuntimeError to have a location. Error message: {message}"));
self.call_stack().last().unwrap_or_else(|| panic!("Expected RuntimeError to have a location. Error message: {message}"));

Diagnostic::simple_error(message, String::new(), location.span)
}
Expand Down
Loading

0 comments on commit fbff386

Please sign in to comment.