Skip to content

Remove witnesses from dist#91

Merged
andrew-fleming merged 1 commit into
OpenZeppelin:mainfrom
andrew-fleming:remove-witnesses-from-dist
May 21, 2026
Merged

Remove witnesses from dist#91
andrew-fleming merged 1 commit into
OpenZeppelin:mainfrom
andrew-fleming:remove-witnesses-from-dist

Conversation

@andrew-fleming
Copy link
Copy Markdown
Contributor

@andrew-fleming andrew-fleming commented May 21, 2026

This PR proposes to remove witnesses from dist/ so that they're not shipped with compact modules

Summary by CodeRabbit

  • Chores
    • Improved the build process to automatically clean up temporary witness directories from the distribution output before finalizing the build artifacts. Added validation tests to ensure this cleanup step executes at the correct point in the build pipeline.

Review Change Stack

@andrew-fleming andrew-fleming requested review from a team as code owners May 21, 2026 06:46
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 203ac24a-e896-4fe3-aa56-a694a9e64b9b

📥 Commits

Reviewing files that changed from the base of the PR and between 81bdff0 and d28dbfc.

📒 Files selected for processing (2)
  • packages/builder/src/Builder.ts
  • packages/builder/test/Builder.test.ts

Walkthrough

This PR adds a witness directory cleanup step to the CompactBuilder pipeline that runs before compact file copying. The implementation uses a find command to recursively remove witnesses directories from dist, with test updates verifying correct step ordering and command execution.

Changes

Witness Directory Removal in Build Pipeline

Layer / File(s) Summary
Witness removal build step implementation
packages/builder/src/Builder.ts
New steps.push entry executes find dist -name "witnesses" -exec rm -rf to delete witness directories, with progress message "Removing witness directories from dist".
Test expectations and witness removal coverage
packages/builder/test/Builder.test.ts
Default pipeline expectations include the witness-removal step; cleanDist: true pipeline length increases from 4 to 5 steps; new test case verifies step exists, command structure matches find dist ... -name "witnesses" -exec rm -rf, and runs after artifact copying but before compact copying; full pipeline expectations updated.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A builder once dreamed in the dust, 🐰
Of witnesses banished, as builders must.
With find and rm, the cleanup takes flight,
Before compacts are copied—all tidy and right!
Step five now removes what should not remain. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Remove witnesses from dist' directly and concisely describes the main change: adding a build step to delete witness directories from the dist directory before copying.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Member

@0xisk 0xisk left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrew-fleming andrew-fleming merged commit 9ed94fa into OpenZeppelin:main May 21, 2026
6 checks passed
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