-
-
Notifications
You must be signed in to change notification settings - Fork 139
Add INSTA_PENDING_DIR environment variable for hermetic builds #852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This adds support for the INSTA_PENDING_DIR environment variable, which allows pending snapshots to be written to a separate directory while keeping the source tree read-only. This is particularly useful for hermetic build systems like Bazel. When INSTA_PENDING_DIR is set: - Pending snapshots (.snap.new and .pending-snap files) are written to the specified directory, preserving the same relative structure as the workspace - cargo-insta correctly discovers and maps these pending snapshots back to their target locations in the source tree - External test paths (e.g., path = "../tests/lib.rs") are rejected with a clear error, as they would escape the pending directory Key changes: - insta: Add get_pending_dir() and pending_snapshot_path() in env.rs - insta: Update runtime.rs to use pending_snapshot_path for all snapshot file operations - insta: Create parent directories in snapshot.rs for pending dir - cargo-insta: Update find_pending_snapshots() in walk.rs to support path mapping between pending_root and target_root - cargo-insta: Simplify load_snapshot_containers() in cli.rs to use unified loop for both hermetic and default modes - cargo-insta: Add comprehensive tests in pending_dir.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
On Windows, paths from different sources may have different formats (e.g., \\?\ prefix from canonicalize vs. regular paths). This caused strip_prefix to fail when comparing workspace and snapshot paths. Fix by canonicalizing both paths before strip_prefix. Fall back to original paths if canonicalization fails (e.g., for test scenarios with non-existent paths). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The previous canonicalization approach failed on Windows because: 1. Canonicalize() fails for non-existent files (new snapshots) 2. When it succeeds, it adds \\?\ extended-length prefix 3. strip_prefix fails when comparing paths with/without the prefix The fix adds normalize_path() helper that: - Tries to canonicalize the full path - Falls back to canonicalizing parent + filename (for env.rs) - Strips the \\?\ prefix on Windows for consistent comparison 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The previous fix didn't handle the case where a non-existent file's parent directories also don't exist yet. On Windows, if part of the path contains 8.3 short names (e.g., RUNNER~1 for runneradmin), canonicalization of the existing workspace directory would convert to long names while the snapshot path stayed in short name form. The fix walks up the path tree to find the first existing ancestor, canonicalizes it (which resolves 8.3 names to long names), then appends the remaining path components. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove the complex path normalization code and try direct strip_prefix. If both paths come from consistent sources, they should match without any normalization. Let's see if Windows CI passes without it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This reverts commit b535d46.
…equired Clarify in doc comments that this complexity exists because Windows paths from different sources have incompatible formats, and without normalization strip_prefix silently fails causing snapshots to go to wrong locations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
I'm testing this on my Bazel example but running into a couple of issues:
env = {
"INSTA_FORCE_PASS": "1",
"INSTA_OUTPUT": "diff",
"INSTA_UPDATE": "new",
"INSTA_PENDING_DIR": "/".join(["$(RULEDIR)", "__{}_snapshots".format(name)]),
},The value of I get the following error:
Note that it detects It detects
The Let me know if I can clarify or test anything further. |
The previous implementation used canonicalize() as a fallback for strip_prefix, but this caused issues with Bazel's symlinked execroot. Testing shows direct strip_prefix works without the normalization fallback. If Windows CI fails, we can add back a targeted fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…allback This preserves symlinks for Bazel's execroot (where direct strip works) while still handling Windows path format differences (where normalization is needed). The key insight is: try direct first, normalize only if that fails. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@kormide have another look at the latest changes; I went back to using I would need to fix for (non-bazel) Windows tests before merging; they seem to not like the symlinks... |
I updated my example in aspect-build/bazel-examples#504 to use your changes and it works great! It also vastly simplified the implementation as I no longer have to rewrite paths or do any magic in the insta wrapper script for reviewing. One nice-to-have would be a way to remove unreferenced snapshots using I'm not sure about the Windows issues, but is it possible that symlinks aren't enabled on your Windows ci runner? IIRC symlinks are opt-in on Windows. |
This test creates a symlinked workspace (like Bazel's execroot) and verifies that INSTA_PENDING_DIR works correctly without following symlinks. This would have caught the bug reported in PR mitsuhiko#852. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Document the limitation that unreferenced snapshot detection requires direct access to the source tree, which doesn't work in Bazel's sandbox. Reference kormide's suggestion for a potential solution using marker files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add backticks around `INSTA_PENDING_DIR` - Wrap URL in angle brackets 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
super! I added the I'll release this and let's see if there's more feedback on this working bazel, let's see how we do thanks for suggesting & testing |
Summary
This adds support for the
INSTA_PENDING_DIRenvironment variable, which allows pending snapshots to be written to a separate directory while keeping the source tree read-only. This is particularly useful for hermetic build systems like Bazel.When
INSTA_PENDING_DIRis set:.snap.newand.pending-snapfiles) are written to the specified directory, preserving the same relative structure as the workspacecargo-instacorrectly discovers and maps these pending snapshots back to their target locations in the source treepath = "../tests/lib.rs") are rejected with a clear error, as they would escape the pending directoryKey changes
get_pending_dir()andpending_snapshot_path()inenv.rsruntime.rsto usepending_snapshot_pathfor all snapshot file operationssnapshot.rswhen pending dir is setfind_pending_snapshots()inwalk.rsto support path mapping betweenpending_rootandtarget_rootload_snapshot_containers()incli.rsto use unified loop for both hermetic and default modespending_dir.rs(9 new tests)Test plan
pending_dirtests cover: file snapshots, inline snapshots, accept, reject, auto-creation, update existing, check mode, outside workspace, and external test path rejection🤖 Generated with Claude Code