fix: add fmt check, fix symlink bug, isolate tools workspaces#3443
Open
ChenxingLi wants to merge 2 commits intoConflux-Chain:masterfrom
Open
fix: add fmt check, fix symlink bug, isolate tools workspaces#3443ChenxingLi wants to merge 2 commits intoConflux-Chain:masterfrom
ChenxingLi wants to merge 2 commits intoConflux-Chain:masterfrom
Conversation
Remaining items from PR Conflux-Chain#3426 after PRs Conflux-Chain#3441 and Conflux-Chain#3442: - test.sh: `rm -f target` → `rm -rf target` — rm -f silently fails on real directories, causing ln -s to create a link inside the directory instead of replacing it - tools/consensus_bench/Cargo.toml: add [workspace] to prevent Cargo from resolving upward to the parent repo's workspace when the worktree is nested inside it - tools/evm-spec-tester/Cargo.toml: same fix, same root cause New: add fmt check to test.sh as a pre-phase before the build. Jenkins Master lacks the nightly toolchain so fmt checking runs on Worker via test.sh. Uses independent anchors (=== Fmt check ===) to avoid renumbering existing phases. Update SKILL.md monitoring guide: pipe-based launch to avoid tee /dev/stderr log corruption, expanded phase regex for fmt anchors, pre-flight fmt check, and fmt phase documentation.
Contributor
|
fmt already checked in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR completes the remaining work from #3426 (after #3441 and #3442) and adds a fmt check phase to
test.sh. The changes fall into two categories: fixes for existing bugs that surface in worktree environments, and a new fmt check that closes a gap in CI coverage.Symlink bug in
check_integration_testsBefore running integration tests,
test.shreplacestargetwith a symlink tobuild/so tests find the compiled binary. The originalrm -f targetcannot remove a real directory — the error is silently ignored, andln -s build targetcreates a symlink inside the surviving directory rather than replacing it. Integration tests then run against a stale or missing binary. Changed torm -rf target.Workspace isolation for
tools/*The main workspace declares
exclude = ["tools"], which works correctly in a normal checkout — Cargo walks up, sees the exclusion, and treats eachtools/*package as standalone.The problem appears in nested worktrees. When a worktree lives under the main repo (e.g.
/conflux-rust/.claude/worktrees/xxx/), Cargo finds the worktree's workspace root first, sees the package is excluded, and continues walking upward — eventually reaching the main repo's workspace. At that level the package's relative path becomes.claude/worktrees/.../tools/<pkg>, which no longer matchesexclude = ["tools"]. Cargo concludes the package is inside a workspace that doesn't claim it, and errors out.Adding
[workspace]toconsensus_bench/Cargo.tomlandevm-spec-tester/Cargo.tomlmakes each package its own workspace root, stopping the upward walk before it escapes the worktree.Fmt check in
test.shThe fmt check was previously assigned to Jenkins Master, which lacks the nightly toolchain — so it was effectively not running. Moving it into
test.shrestores the CI gate on Worker where the toolchain is available, and benefits developers who usetest.shas a local pre-commit check.The check runs before Phase 1 (build) with independent anchors (
=== Fmt check ===/=== Fmt check passed ===) to avoid renumbering the existing four phases.SKILL.md monitoring updates
test.sh's check functions use
tee /dev/stderrto echo captured output. When the log is collected via> file 2>&1, fd 2 points to a regular file, andtee'sopen("/proc/self/fd/2")creates a new file description at position 0 — overwriting earlier log content including phase anchors. The fix: the launch command now pipes throughtee(2>&1 | tee file), so fd 2 inside test.sh is a pipe. Pipes have no seek position, eliminating the overwrite.The monitoring guide also picks up the new fmt phase: the detection regex recognizes fmt anchors, and a pre-flight fmt check is added so formatting issues surface before the long build. The Phase 2 workspace resolution warning is removed, as the
[workspace]fix above eliminates the underlying risk.Test plan
Tested in a nested worktree at
/conflux-rust/.claude/worktrees/fix-remaining-3426/.Workspace isolation:
cargo_fmt.sh -- --checkhit the workspace resolution error on bothconsensus_benchandevm-spec-testerbefore the fix, and passed after.Fmt monitoring: Launched test.sh via pipe with an artificially introduced fmt failure. The monitoring regex correctly detected
Fmt checkas the current phase and observed process exit before Phase 1. After reverting the failure, confirmed the phase transition fromFmt check passedtoPhase 1/4.Not yet tested: full end-to-end
test.sh(build + integration + pytest) requires a clean build on Worker.This change is