fix(js_image_layer): support js_image_layer in local_path_override (bzlmod nested) workspaces#2826
Open
menny wants to merge 3 commits intoaspect-build:mainfrom
Open
fix(js_image_layer): support js_image_layer in local_path_override (bzlmod nested) workspaces#2826menny wants to merge 3 commits intoaspect-build:mainfrom
menny wants to merge 3 commits intoaspect-build:mainfrom
Conversation
…_external (aspect-build#2824) Files from local_path_override modules have f.owner.repo_name != "", which caused is_external=true and forced them through _mtree_file_line using the dest path directly. On remote execution the dest path is a sandbox symlink that does not exist, producing an ENOENT / mtree type-mismatch error. Fix: drop the is_external shortcut so all node_modules symlinks go through resolveSymlink. When the resolved path has no reverse-map entry (e.g. a package-store directory), fall back to computing the runfiles linkname using the same logic as _to_rlocation_path in js_image_layer.bzl: strip "external/" for external repos, prepend REPO_NAME for the main workspace.
- Remove is_external from the _ENTRY format string (.bzl) and from the Entry typedef, destructuring, and condition check (.mjs); the field is no longer needed after the is_external fix. - Replace `+ 1` with `.replace(/^\//, '')` when stripping the leading slash separator in pathAfterRoot, making the intent explicit. - Guard against realp.indexOf(root) returning -1 by throwing early with a descriptive error before any slice is attempted; repurposes the previously unreachable linkname==undefined error block.
The loop over expanded tree files was mutating the outer `runfiles_dest` and `path` variables with `+=`. This caused two bugs: - `runfiles_dest` accumulated file names across iterations without a `/` separator, producing paths like `dir/file1.jsfile2.js`. - `path` was already JSON-encoded (surrounding quotes), so appending a raw string produced invalid JSON like `"dir/path"file.js`. Use iteration-local `file_dest` and `file_path` instead, computing each file's dest by appending `tree_relative_path` with a `/`, and encoding the individual file's path fresh each iteration.
|
Member
|
Can we add a test/example that demonstrates the bug+fix? |
Author
|
i've linked to a branch with a commit showing the error. The commit message explains how to reproduce. I'll add a summary here in the description. |
Member
|
Can we add a test though? A test that previously failed and now passes with your fix... |
Author
gladly, I am not sure how to create that test though :) This seems like an integration test? Can you direct me to such tests I can pattern match on? |
Member
|
If the bug involves a Ideally we don't need to add another e2e test and we can just modify |
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
2 participants
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.
Fixes #2824
Problem
js_image_layerfails when the target lives in a module registered vialocal_path_overrideand is built from the outer workspace.Files from such modules have
f.owner.repo_name != "", which causedis_external = truein the manifest entries. Theis_externalflag short-circuited symlink resolution and wrotenode_modules/entries directly astype=filein the mtree using thedestpath. Thatdestis a sandbox symlink, not a regular file - causing a type mismatch error from bsdtar.Fix
Drop the
is_externalfield entirely. Allnode_modules/entries now go throughresolveSymlink. When the resolved path has no reverse-map entry (e.g. a package-store directory symlink likenode_modules/acorn → .aspect_rules_js/acorn@8.7.1/node_modules/acorn), fall back to computing the runfiles linkname using the same logic as_to_rlocation_pathinjs_image_layer.bzl:bazel-out/<cfg>/bin/external/<repo>/...→<repo>/...bazel-out/<cfg>/bin/<pkg>/...→REPO_NAME/<pkg>/...Additional fixes included
indexOfreturning -1:realp.indexOf(root)now fails fast with a descriptive error ifrootis not found in the resolved path, rather than silently corrupting slice indices.runfiles_destandpathvariables with+=, producing path concatenation without/separators and appending raw strings onto already-JSON-encoded values. Fixed by using iteration-localfile_dest/file_path.Repro
See repro in b7d1b8b
Repro structure:
Building from
child/folder:bazel build //:layerwill work just fine.Building from the root
js_image_layer_is_externalfolder:bazel build @child//:layerwill fail with: