Skip to content

node-proto-build - better protox errors#1213

Merged
drahnr merged 5 commits intonextfrom
bernhard-better-protox-errors
Sep 10, 2025
Merged

node-proto-build - better protox errors#1213
drahnr merged 5 commits intonextfrom
bernhard-better-protox-errors

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented Sep 9, 2025

Currently we reduce miette::Error to anyhow::Error. This retains the miette context, upgrading output from:


Caused by:
  process didn't exit successfully: `/media/humpback/projects/miden/miden-node-bernhard-1185-acounts/target/debug/build/miden-node-proto-build-c8b64ef83bec5c7c/build-script-build` (exit status: 1)
  --- stdout
  cargo::rerun-if-changed=./proto
  cargo::rerun-if-env-changed=BUILD_PROTO

  --- stderr
  [proto/build.rs:25:5] color_eyre::install() = Ok(
      (),
  )
  [proto/build.rs:26:5] run() = Err(
      store/rpc.proto:110:19: expected an identifier, but found '{',
  )
  Error: 
     0: expected an identifier, but found '{'

  Location:
     proto/build.rs:43

to:

  process didn't exit successfully: `/media/humpback/projects/miden/miden-node-bernhard-1185-acounts/target/debug/build/miden-node-proto-build-45f1c9d2f9c973e5/build-script-build` (exit status: 1)
  --- stdout
  cargo::rerun-if-changed=./proto
  cargo::rerun-if-env-changed=BUILD_PROTO

  --- stderr
  [proto/build.rs:25:5] run() = Err(
      store/rpc.proto:119:39: field number '2' is already used,
  )
  Error:   × field number '2' is already used
       ╭─[store/rpc.proto:117:36]
   116 │                 // (e.g., with fewer than 1000 entries).
   117 │                 bool all_entries = 2;
       ·                                    ┬
       ·                                    ╰── first defined here
   118 │ 
   119 │                 AllMapKeys map_keys = 2;
       ·                                       ┬
       ·                                       ╰── defined again here
   120 │             }
       ╰────

  Error: 
    × name 'Digest' is not defined
       ╭─[store/rpc.proto:109:26]
   108 │                 // A list of map keys (Digests) associated with this storage slot.
   109 │                 repeated Digest map_keys = 1;
       ·                          ───┬──
       ·                             ╰── found here
   110 │             }
       ╰────

  Error: 
    × name 'AccountWitness' is not defined
       ╭─[store/rpc.proto:168:5]
   167 │     // Account ID, current state commitment, and SMT path
   168 │     AccountWitness witness = 1;
       ·     ───────┬──────
       ·            ╰── found here
   169 │ 
       ╰────

  Error: 
    × name 'AccountHeader' is not defined
       ╭─[store/rpc.proto:155:9]
   154 │         // Account header.
   155 │         AccountHeader header = 1;
       ·         ──────┬──────
       ·               ╰── found here
   156 │ 
       ╰────

  Error: 
    × name 'Digest' is not defined
       ╭─[store/rpc.proto:194:13]
   193 │             // a `Word` representing the commitment
   194 │             Digest commitment = 2;
       ·             ───┬──
       ·                ╰── found here
   195 │         }
       ╰────

make: *** [Makefile:88: build] Error 101

@drahnr drahnr added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Sep 9, 2025
Copy link
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally not a fan of miette for conventional error chaining because its so verbose. Its great for error formatting within generated code but less so for simple .context("failed xyz").

Am I correct in saying your two examples are from different errors because they don't really line up?

I don't have a strong opinion on the error handling here as we don't really expect this to ever error for users; and very rarely for devs.

@drahnr
Copy link
Contributor Author

drahnr commented Sep 10, 2025

Am I correct in saying your two examples are from different errors because they don't really line up?

Correct. Although if there a multiple errors, only one would be shown with anyhow::Result.

I don't have a strong opinion on the error handling here as we don't really expect this to ever error for users; and very rarely for devs.

An actionable error message is always a win in my books. Particularly when editing proto-files and getting the exact error location compared to a too-small-to-search-for snippet, it becomes a shortcut to running protoc manually with the correct arguments.

@Mirko-von-Leipzig
Copy link
Collaborator

I don't have a strong opinion on the error handling here as we don't really expect this to ever error for users; and very rarely for devs.

An actionable error message is always a win in my books. Particularly when editing proto-files and getting the exact error location compared to a too-small-to-search-for snippet, it becomes a shortcut to running protoc manually with the correct arguments.

Makes sense :) Thanks!

@drahnr drahnr merged commit 8d68ac1 into next Sep 10, 2025
6 checks passed
@drahnr drahnr deleted the bernhard-better-protox-errors branch September 10, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants