fix(compile): respect npm registry sub-paths when flattening node_modules#34575
Conversation
…ules `deno compile` flattens each cached npm registry's package directories into `.deno_compile_node_modules/localhost/<pkg>/...` so the embedded binary can serve them from a single dummy registry at runtime. The flattening only descended one directory level, which works when the registry URL has no path (e.g. `https://registry.npmjs.org/`) but skips a level for registries whose URL includes a sub-path (e.g. `http://mirrors.example.com/npm/`). Packages end up at `localhost/<sub>/<pkg>/...` and the compiled binary fails at runtime with `ERR_MODULE_NOT_FOUND`. Use the known registry dir-names already tracked by `NpmCacheDir` (built from `.npmrc` resolution) to walk to each registry's actual package root before flattening, and only drop the registry host directories whose packages were lifted into `localhost/`. The default no-sub-path case behaves identically. Fixes denoland#26628
Teaches the test npm registry server to strip a fixed `/sub/path` prefix before serving package files, so tests can exercise a registry url with a sub-path. Adds a compile spec that caches a package through such a registry and asserts the embedded layout flattens to `localhost/<pkg>` rather than `localhost/sub/path/<pkg>` (regression test for denoland#26628).
|
Added the deferred spec test in f00bebd:
Verified locally that the new spec passes and that the existing |
On Windows the compiled output is main.exe, so the embedded-files tree root is main.exe rather than main. Use main[WILDLINE] like the other compile specs so the assertion matches on all platforms.
lunadogbot
left a comment
There was a problem hiding this comment.
(Comment review — can't approve as same auth account.) LGTM. Using NpmCacheDir's already-tracked known_registries_dirnames is the right hook — the cache layout is the source of truth, so deriving the flatten paths from it instead of assuming one-level depth fixes the regression without heuristics. The two new pass-throughs (NpmCacheDir::known_registries_dirnames, ManagedCliNpmResolver::known_registries_dirnames) are minimal.
Substantive notes:
-
registry_top_segmentscollects the first path segment of each known registry dirname and then drops any top-level dir under the global cache whose name matches. If two registries share the same host (e.g.localhost:4873/aandlocalhost:4873/b), only their explicitregistry_dirnamesubtrees are flattened intolocalhost_entries, but the entirelocalhosttop-level dir gets dropped. If a host dir ever contained packages cached under an unknown registry path on that same host, those would silently be lost. Realistically the global cache only gets populated through known registries, so I don't think this can happen — but a one-line comment in the new block explaining the assumption would save the next reader some time. -
The duplicate-entry
panic!is preserved from the previous code (so this PR doesn't make it worse), but with multiple registries flattening into the samelocalhost_entriesmap, the surface area for a real-world collision (e.g. same scoped package cached from two mirrors) is now higher. Not a blocker, but if a user reports a panic here in the wild, this is the place.
The handle_req_for_registry /sub/path strip is fine for the test; the spec asserts both the embedded layout (.deno_compile_node_modules/localhost/@denotest/esm-basic/*, not localhost/sub/path/...) and runtime resolution. Good regression guard.
…ules (denoland#34575) Fixes denoland#26628 Co-authored-by: Bartek Iwańczuk <[email protected]>
Summary
Fixes #26628.
deno compileproduced binaries that failed withERR_MODULE_NOT_FOUNDwhen the configured npm registry URL contained a sub-path (e.g.http://mirrors.example.com/npm/).The on-disk cache layout for such a registry is
<global_cache>/<host>/<sub>/<pkg>/..., butbuild_vfs_consolidating_global_npm_cacheincli/standalone/binary.rsflattened the cache by iterating each top-level entry under<global_cache>exactly one level deep — collapsing<global_cache>/<host>/<pkg>directly into the embeddedlocalhost/. With a sub-path registry the loop instead picks up the intermediate<sub>directory and embeds packages atlocalhost/<sub>/<pkg>/..., which then can't be found at runtime (where the binary expectslocalhost/<pkg>/...).This PR uses the
known_registries_dirnamesalready tracked byNpmCacheDir(built from.npmrcresolution) to walk to each registry's actual package root before flattening, and to know which top-level host directories under the global cache are registry roots that should be dropped after their packages are moved intolocalhost/. The default no-sub-path case is unchanged.To make this accessible from
cli/standalone/binary.rs, the PR adds:NpmCacheDir::known_registries_dirnames()ManagedCliNpmResolver::known_registries_dirnames()Manual repro
I verified end-to-end with a Deno-script proxy that mirrors
registry.npmjs.orgat a sub-path:Before this PR:
After:
I also re-checked the default (no sub-path) registry case end-to-end to confirm no regression — embedded layout and runtime behaviour are unchanged.
A spec test is now included.
handle_req_for_registryintests/util/server/servers/npm_registry.rsstrips a fixed/sub/pathprefix before serving package files, so the test registry can be driven through a sub-path url. The spec attests/specs/compile/npm_registry_subpath/compiles a package cached throughhttp://localhost:4260/sub/path/and asserts the embedded layout flattens tolocalhost/@denotest/esm-basic/*rather thanlocalhost/sub/path/..., then runs the binary to confirm it resolves at runtime.Test plan
compile/npm_registry_subpathspec asserts the flattened layout and runtime behaviour