Pin shell in installer-generated env setup (Schniz#1525) #204
Pin shell in installer-generated env setup (Schniz#1525) #204Dargon789 wants to merge 10 commits intoDargon789:Dargon789-patch-9from
Conversation
* fix(zsh): dedupe use-on-cd hook on re-source * fix(wincmd): support spaces and drive switches in use-on-cd * test(e2e): cover use-on-cd after re-sourcing env * chore: add changeset for use-on-cd shell fixes
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* feat: support x64-glibc-217 * add changeset --------- Co-authored-by: Gal Schlezinger <[email protected]>
* fix(use): clarify interactive install prompt source * chore: add changeset for prompt clarification
* perf(install): pin shell in generated env setup * ci: pin docker platform for ARM installer tests * chore: add changeset for installer shell update
Reviewer's GuidePins the shell used by the installer-generated env setup, optimizes Sequence diagram for fnm env --use-on-cd applying initial versionsequenceDiagram
actor User
participant Shell
participant Fnmbin as fnm_binary
participant EnvCmd as Env_command
participant UseCmd as Use_command
participant OS as OS_environment
User->>Shell: evaluate env setup (fnm env --use-on-cd --shell <shell>)
Shell->>Fnmbin: invoke Env subcommand
Fnmbin->>EnvCmd: run Env.apply(config)
EnvCmd->>OS: compute multishell_path
EnvCmd->>OS: set_path_for_multishell(multishell_path)
OS-->>EnvCmd: PATH updated to include multishell bin
EnvCmd->>EnvCmd: config_with_multishell = config.with_multishell_path(multishell_path)
EnvCmd->>UseCmd: construct Use_command(info_to_stderr = true, install_if_missing = false, silent_if_unchanged = true)
EnvCmd->>OS: check NO_COLOR, stdout, stderr
alt should_force_stderr_color
EnvCmd->>OS: enable colored override
end
EnvCmd->>UseCmd: Use.apply(config_with_multishell)
UseCmd->>OS: read version file and select Node version
UseCmd->>OS: update PATH for selected Node version
UseCmd-->>EnvCmd: result (errors ignored)
alt should_force_stderr_color
EnvCmd->>OS: disable colored override
end
EnvCmd->>Shell: print shell hook script (shell.use_on_cd(config))
Shell-->>User: hook installed without extra fnm use subprocess on first cd
Updated class diagram for FnmConfig, Use, Arch, and related typesclassDiagram
class FnmConfig {
<<clap::Parser, Clone>>
+mirror Url
+arch Arch
+multishell_path Option~PathBuf~
+with_base_dir(base_dir Option~PathBuf~) FnmConfig
+with_multishell_path(multishell_path PathBuf) FnmConfig
}
class Directories {
<<Clone>>
+base BaseStrategy
+new() Directories
+cache_dir() PathBuf
}
class Use {
+version Option~UserVersion~
+install_if_missing bool
+silent_if_unchanged bool
+info_to_stderr bool
+apply(config &FnmConfig) Result~(), Error~
}
class Arch {
<<enum>>
X86
X64
X64Musl
X64Glibc217
Arm64
Armv7l
Ppc64le
Arm64Musl
+to_string() &str
+from_str(s &str) Result~Arch, Error~
}
class EnvCommand {
+use_on_cd bool
+apply(config &FnmConfig) Result~(), Error~
-set_path_for_multishell(multishell_path &Path)
}
class Shell {
<<trait>>
+env(config &FnmConfig) String
+use_on_cd(config &FnmConfig) Result~String, Error~
+rehash() Option~String~
}
class Zsh {
+use_on_cd(config &FnmConfig) Result~String, Error~
-add_zsh_hook_remove_existing()
}
class Bash {
+use_on_cd(config &FnmConfig) Result~String, Error~
}
class Fish {
+use_on_cd(config &FnmConfig) Result~String, Error~
}
class PowerShell {
+use_on_cd(config &FnmConfig) Result~String, Error~
}
class WindowsCmd {
+use_on_cd(config &FnmConfig) Result~String, Error~
+create_cd_file_at(path &Path) Result~(), IoError~
}
FnmConfig *-- Directories
FnmConfig o-- Arch
EnvCommand ..> Use : constructs
EnvCommand ..> FnmConfig : reads and clones
Use ..> FnmConfig : uses
Zsh ..|> Shell
Bash ..|> Shell
Fish ..|> Shell
PowerShell ..|> Shell
WindowsCmd ..|> Shell
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
set_path_for_multishell, theunsafeblock aroundstd::env::set_varis unnecessary sinceset_varis safe; you can drop theunsafeand simplify the function. - In
windows_cmdtests, consider using atempfile::TempDir(or similar RAII temp dir) and avoidingunwrap()on cleanup calls so tests don't panic or leave stray directories/files behind ifcd.cmdisn't created as expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `set_path_for_multishell`, the `unsafe` block around `std::env::set_var` is unnecessary since `set_var` is safe; you can drop the `unsafe` and simplify the function.
- In `windows_cmd` tests, consider using a `tempfile::TempDir` (or similar RAII temp dir) and avoiding `unwrap()` on cleanup calls so tests don't panic or leave stray directories/files behind if `cd.cmd` isn't created as expected.
## Individual Comments
### Comment 1
<location path="docs/commands.md" line_range="383" />
<code_context>
This command generates a series of shell commands that should be evaluated by your shell to create a fnm-ready environment.
-Each shell has its own syntax of evaluating a dynamic expression. For example, evaluating fnm on Bash and Zsh would look like `eval "$(fnm env)"`. In Fish, evaluating would look like `fnm env | source`
+Each shell has its own syntax of evaluating a dynamic expression. For example, evaluating fnm on Bash and Zsh would look like `eval "$(fnm env --shell bash)"`. In Fish, evaluating would look like `fnm env --shell fish | source`
Usage: fnm env [OPTIONS]
</code_context>
<issue_to_address>
**suggestion (typo):** Use more idiomatic phrasing "syntax for evaluating" instead of "syntax of evaluating".
Please change the sentence to: "Each shell has its own syntax for evaluating a dynamic expression."
```suggestion
Each shell has its own syntax for evaluating a dynamic expression. For example, evaluating fnm on Bash and Zsh would look like `eval "$(fnm env --shell bash)"`. In Fish, evaluating would look like `fnm env --shell fish | source`
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request updates fnm to version 1.39.0, adding support for the x64-glibc-217 architecture and optimizing shell startup for env --use-on-cd by applying the initial version internally. It also improves shell hook robustness for Zsh and Windows CMD, adds a use flag to the install command, and clarifies the missing-version prompt. Feedback suggests adding safety documentation for the unsafe environment variable modification and refactoring the output utility to handle stderr redirection more cleanly than repurposing the Error log level.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Head branch was pushed to by a user without write access
* feat: add possible values to the arch help docs. * add a changeset --------- Co-authored-by: Gal Schlezinger <[email protected]>
|
@theborowski is attempting to deploy a commit to the Foundry development Team on Vercel. A member of the Team first needs to authorize it. |
* docs: add e2e testing guide for agents * docs: trim AGENTS e2e section * docs: refine AGENTS guidance for e2e and changesets
* Add random number to multishell symlinks This protects against collisions when multiple shells are started in parallel inside Bubblewrap sandboxes, as each shell will have the same timestamp and the same low PID (e.g. 2), since Bubblewrap sandboxes have isolated PID spaces. * Add a changeset --------- Co-authored-by: Gal Schlezinger <[email protected]>
Pin shell in installer-generated env setup (https://github.com/Schniz/fnm/pull/1525[)](https://github.com/Dargon789/fnm/commit/bfb186034978b1ddaf87501eb1633bdc42a5c0a6)
perf(install): pin shell in generated env setup
ci: pin docker platform for ARM installer tests
chore: add changeset for installer shell update
Summary by Sourcery
Optimize
fnm env --use-on-cdshell setup, harden shell hooks across platforms, and update installer, architecture support, and docs accordingly.New Features:
x64-glibc-217architecture via the--arch x64-glibc-217option infnm env.Bug Fixes:
--use-on-cdhooks work with paths containing spaces and handle drive changes correctly.--use-on-cdhook when shell configuration is re-sourced.Enhancements:
fnm env --use-on-cdby applying the current directory Node version duringfnm envexecution instead of via an extrafnm usesubprocess.--shellflags in installer-generated shell setup to avoid runtime shell inference and process tree detection.fnm useoutput to stderr when invoked internally fromfnm envto keep stdout clean for shell evaluation.fnm useinteractive missing-version prompt by prefixing the message withfnmfor clearer attribution.Build:
CI:
--platformfor ARM installer tests in the installation script CI workflow and normalize YAML formatting.Documentation:
--shelltofnm envand update examples in CLI docs and README accordingly.Tests:
--use-on-cdenv setup across shells.Chores: