Repository navigation
fix(install): rewrite node_modules .bin shim generation for windows to be like npm - #31494
Conversation
WalkthroughAdds Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/npm_installer/bin_entries.rs (1)
430-436: Consider renaming this macro to avoid shadowingstd::writeln!.The macro works correctly since writing to a
Stringis infallible, but shadowing the standard library'swriteln!can be confusing for future maintainers. A name likewriteln_unchecked!or a brief comment explaining the rationale would help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
libs/npm_installer/Cargo.toml(1 hunks)libs/npm_installer/bin_entries.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
libs/npm_installer/bin_entries.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test debug macos-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test release windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (8)
libs/npm_installer/Cargo.toml (1)
38-38: LGTM!The
lazy-regexdependency is appropriate for the shebang parsing logic inbin_entries.rs. It provides compile-time regex validation which is preferable to runtime regex compilation.libs/npm_installer/bin_entries.rs (7)
3-20: LGTM!Proper attribution and license inclusion for the ported npm/cmd-shim code.
446-467: LGTM!The shebang parsing regex is correctly ported from npm/cmd-shim and the implementation handles the various shebang formats (env-based, direct path, with args/vars) properly. Good test coverage validates the behavior.
469-477: LGTM!Efficient first-line-only reading for shebang detection. Treating all errors as "no shebang" is appropriate since a missing or unreadable file will be handled elsewhere.
787-1086: LGTM!Comprehensive test coverage for shebang parsing and all three shim formats. The
trim_indenthelpers make assertions readable. Good job covering edge cases like env vars and arguments.
716-718: LGTM!Simple wrapper around
pathdiff::diff_paths.
408-416: Verify platform-specific parameter usage inset_up_bin_shim.The concern about removing
#[allow(unused_variables)]requires confirming whether thepackageandextraparameters are actually unused whenset_up_bin_shimis called on Windows. If these parameters remain unused after the refactoring, compiler warnings would occur without the attribute.
571-575: Double spaces in exec statements are intentional npm/cmd-shim behavior and functionally harmless.Lines 572 and 574 may produce output like
exec NODE_ENV=prod "$basedir/node" "--experimental" ...whenvarsis non-empty. These double spaces come from how npm's cmd-shim template joins optional arguments and environment variables. POSIX shells treat multiple consecutive spaces as a single separator, so this spacing has no functional impact and is expected cross-platform compatibility behavior.
| }) | ||
| } | ||
|
|
||
| fn resolve_shebang(sys: &impl FsOpen, path: &Path) -> Option<Shebang> { |
There was a problem hiding this comment.
We have a separate
...maybe the two could be consolidated?
There was a problem hiding this comment.
It feels a little weird for deno_npm_installer to depend on deno_task_shell but maybe fine
There was a problem hiding this comment.
Hmmm I didn't think about it much. Maybe this could be feature flagged in that crate? I'm actually not sure it's worth consolidating btw. It might be though.
There was a problem hiding this comment.
Yeah I'm not sure it's worth it. I think I'd prefer to land this as is to unblock dx, and then maybe consolidate
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/npm_installer/bin_entries/windows_shim.rs (1)
307-308: HandleNonefromrelative_pathinstead of panicking.The
unwrap()can panic ifpathdiff::diff_pathsreturnsNone(e.g., different drive letters on Windows). This was flagged in a previous review.
🧹 Nitpick comments (2)
libs/npm_installer/bin_entries/windows_shim.rs (1)
233-244: Inconsistent indentation in generated PowerShell for the None branch.The
Nonebranch produces a script where theif ($MyInvocation.ExpectingInput)block is indented 2 extra spaces relative to the surrounding code, while theSomebranch starts$ret=0at the base level. This creates inconsistent output between the two cases.match &shebang_data { None => { - builder.append(" if ($MyInvocation.ExpectingInput) {\n"); - builder.append(" $input | & \"$basedir/"); + builder.append("if ($MyInvocation.ExpectingInput) {\n"); + builder.append(" $input | & \"$basedir/"); builder.append(&self.target); - builder.append("\" $args\n"); - builder.append(" } else {\n"); - builder.append(" & \"$basedir/"); + builder.append("\" $args\n"); + builder.append("} else {\n"); + builder.append(" & \"$basedir/"); builder.append(&self.target); - builder.append("\" $args\n"); - builder.append(" }\n"); + builder.append("\" $args\n"); + builder.append("}\n"); builder.append("exit $LASTEXITCODE\n");Note: This would require updating the corresponding test expectations as well.
libs/npm_installer/bin_entries.rs (1)
389-397: Redundant return after?operator.
set_up_bin_shimalready returnsResult<EntrySetupOutcome, BinEntriesError>. Using?extracts the outcome but then you discard it and returnOk(EntrySetupOutcome::Success)again.if sys_traits::impls::is_windows() { - windows_shim::set_up_bin_shim( + return windows_shim::set_up_bin_shim( sys, bin_name, bin_script, package_path, bin_node_modules_dir_path, - )?; - Ok(EntrySetupOutcome::Success) + ); } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/npm_installer/bin_entries.rs(3 hunks)libs/npm_installer/bin_entries/windows_shim.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
libs/npm_installer/bin_entries/windows_shim.rslibs/npm_installer/bin_entries.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
libs/npm_installer/bin_entries/windows_shim.rslibs/npm_installer/bin_entries.rs
🧬 Code graph analysis (2)
libs/npm_installer/bin_entries/windows_shim.rs (1)
libs/npm_installer/bin_entries.rs (1)
relative_path(469-471)
libs/npm_installer/bin_entries.rs (2)
libs/npm_installer/bin_entries/windows_shim.rs (1)
set_up_bin_shim(297-327)libs/npm_installer/local.rs (1)
relative_path(1352-1354)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (5)
libs/npm_installer/bin_entries/windows_shim.rs (3)
41-62: Shebang parsing looks correct.The regex pattern handles the common shebang formats well (direct paths,
/usr/bin/env, and-Sflag with environment variables). Good use oflazy_regexfor compile-once semantics.
64-72: LGTM.Silently returning
Noneon file open/read errors is appropriate here since shebang is optional and the shim will work without it.
329-628: Good test coverage.Tests comprehensively cover shebang parsing edge cases and all three shim formats. The
trim_indenthelpers make expected values readable.libs/npm_installer/bin_entries.rs (2)
3-4: Good modular separation.Moving the Windows shim logic to a separate module cleanly separates the ISC-licensed code from npm/cmd-shim.
469-471: Extractrelative_pathhelper to a shared utility module.This function duplicates an identical definition in
libs/npm_installer/local.rs. Consolidating to a shared utility would eliminate duplication and improve maintainability.
Previously we were just generating a little cmd file that did
deno run npm:/....., but we were the odd one out in that most other package managers do .cmd, a sh script, and a powershell script.This ports the shim logic from https://github.com/npm/cmd-shim and uses that instead.
Parts of the port were done by claude, mostly the string handling nonsense and the tests