Skip to content

fix: handle workspace members in needsRecompile crate collection#21284

Merged
benesjan merged 2 commits intomerge-train/fairiesfrom
jan/fix-needs-recompile-workspaces
Mar 11, 2026
Merged

fix: handle workspace members in needsRecompile crate collection#21284
benesjan merged 2 commits intomerge-train/fairiesfrom
jan/fix-needs-recompile-workspaces

Conversation

@benesjan
Copy link
Contributor

@benesjan benesjan commented Mar 10, 2026

Summary

  • Fix collectCrateDirs to handle Nargo workspace roots (those with [workspace] and members defined)
  • Previously, when the root Nargo.toml was a workspace, its member crates were never visited, so path-based dependencies declared in member crates were not tracked for recompilation
  • Now workspace members are visited recursively, and their path-based dependencies are followed as expected
  • Added a test covering the workspace scenario with members that have path dependencies to external libs

Test plan

  • Unit test traverses workspace members and their path dependencies added and passing
  • All existing needsRecompile tests continue to pass (15/15)

🤖 Generated with Claude Code

Copy link
Contributor Author

benesjan commented Mar 10, 2026

Copy link
Contributor

@nchamo nchamo left a comment

Choose a reason for hiding this comment

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

Have a small question about members + dependencies and if they can co-exist


const members = (parsed.workspace as Record<string, any>)?.members as string[] | undefined;

if (Array.isArray(members)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious. Is it possible for a workspace root to have both members and dependencies? If not, could we clarify that in a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workspace is a collection of crates and doesn't have its own source code.

I don't think this is the place to explain what is noir workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not? In this code you are assuming that the toml file either has members or dependencies. I'm not saying that you should write a full explanation of how Noir workspaces work, but a one line doc that explains that this is the actual behavior would benefit people not yet familiar with Noir

But feel free to ignore

Comment on lines +267 to +275
const testToml = `[package]
name = "test_test"
type = "lib"

[dependencies]
aztec = { git = "https://github.com/AztecProtocol/aztec-nr", tag = "v5.0.0" }
ext = { path = "../external_lib" }
test_contract = { path = "../test_contract" }
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make it easier to read like this? And we could do the same with the other definitions above and below

Suggested change
const testToml = `[package]
name = "test_test"
type = "lib"
[dependencies]
aztec = { git = "https://github.com/AztecProtocol/aztec-nr", tag = "v5.0.0" }
ext = { path = "../external_lib" }
test_contract = { path = "../test_contract" }
`;
const testToml = `
[package]
name = "test_test"
type = "lib"
[dependencies]
aztec = { git = "https://github.com/AztecProtocol/aztec-nr", tag = "v5.0.0" }
ext = { path = "../external_lib" }
test_contract = { path = "../test_contract" }
`;

Copy link
Contributor Author

@benesjan benesjan Mar 11, 2026

Choose a reason for hiding this comment

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

Then the resulting Nargo.toml would start with a new line. I don't think this matters much as it's already not hard to read.

@benesjan benesjan marked this pull request as draft March 11, 2026 02:37
benesjan and others added 2 commits March 11, 2026 02:42
When a Nargo.toml defines a [workspace] with members, collectCrateDirs
now visits each member directory and follows their path-based
dependencies. Previously only the root Nargo.toml's [dependencies]
were checked, which meant workspace member deps were never discovered
and source changes in them would not trigger recompilation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
@benesjan benesjan force-pushed the jan/fix-needs-recompile-workspaces branch from 1ecd98d to e79fc61 Compare March 11, 2026 02:49
@benesjan benesjan marked this pull request as ready for review March 11, 2026 02:49
@benesjan benesjan requested a review from nchamo March 11, 2026 02:49
@benesjan benesjan merged commit f319231 into merge-train/fairies Mar 11, 2026
16 checks passed
@benesjan benesjan deleted the jan/fix-needs-recompile-workspaces branch March 11, 2026 13:15
benesjan added a commit that referenced this pull request Mar 11, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 13, 2026
BEGIN_COMMIT_OVERRIDE
fix: skip oracle version check for pinned protocol contracts (#21349)
fix: not reusing tags of partially reverted txs (#20817)
feat: move storage_slot from partial commitment to completion hash
(#21351)
feat: offchain reception (#20893)
fix: handle workspace members in needsRecompile crate collection
(#21284)
fix(aztec-nr): return Option from decode functions and fix event
commitment capacity (#21264)
fix: handle bad note lengths on compute_note_hash_and_nullifier (#21271)
fix: address review feedback from PRs #21284 and #21237 (#21369)
fix: claim contract & improve nullif docs (#21234)
feat!: auto-enqueue public init nullifier for contracts with public
functions (#20775)
fix: search for all note nonces instead of just the one for the note
index (#21438)
fix: set anvilSlotsInAnEpoch in e2e_offchain_payment to prevent
finalization race (#21452)
fix: complete legacy oracle mappings for all pinned contracts (#21404)
fix: correct inverted constrained encryption check in message delivery
(#21399)
feat!: improve L2ToL1MessageWitness API (#21231)
END_COMMIT_OVERRIDE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants