Skip to content

Fix: Use builtin cd for nvm_cd function#3836

Open
glensc wants to merge 3 commits into
nvm-sh:masterfrom
glensc:patch-1
Open

Fix: Use builtin cd for nvm_cd function#3836
glensc wants to merge 3 commits into
nvm-sh:masterfrom
glensc:patch-1

Conversation

@glensc

@glensc glensc commented May 3, 2026

Copy link
Copy Markdown
Contributor

Use a more reliable built-in command invocation.

Fixes #3835

The previous fix 942e9ab (#1284) was not sufficient.

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either way, this would need a regression test.

Comment thread nvm.sh Outdated
@glensc

glensc commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

Seems the builtin is not available everywhere:


+ builtin cd /workspace/.cache/src/node-v0.10.7/files
./install from source: 33: ./install from source: builtin: not found

Alternative fixes:

  1. use chdir, less commonly overwritten
  2. use more detection, unlikely \cd and builtin both unavailable

@ljharb

ljharb commented May 3, 2026

Copy link
Copy Markdown
Member

if there's no universal posix-compliant way to bypass functions, then there's not much that can be done - can you modify your cd function so it takes the same options as cd?

@glensc

glensc commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

I also thought about unset cd, but can't do that, affects the effective shell for the user....

@glensc

glensc commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

seems command cd works on some systems, even though it's supposedly executing an external command, it seems to execute a builtin

macOS 10.15.8

$ bash -c 'command cd /; pwd'
/

$ sh -c 'command cd /; pwd'
/

$ zsh -c 'command cd /; pwd'
/Users/glen

Debian 12

$ bash -c 'command cd /; pwd'
/

$ sh -c 'command cd /; pwd'
/

$ zsh -c 'command cd /; pwd'
bash: zsh: command not found

@glensc

glensc commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

I propose this version as next:

nvm_cd() {
  if command -v builtin >/dev/null 2>&1; then
    builtin cd "$@"
  else
    \cd "$@"
  fi
}

WDYT?

@ljharb

ljharb commented May 3, 2026

Copy link
Copy Markdown
Member

It'd be nice to define one of two functions conditionally rather than always have to check for builtin (penalizing the 99.99999% case where it's not needed), but in general that seems like it should work.

@glensc

glensc commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

I did think about conditional function definition, but as the rest of the code doesn't do anything like this, I proposed to skip that part.

Very likely, defining a function conditionally causes some incompatibility issue, perhaps there's a shell that doesn't allow it, i.e., it considers such code as function re-definition.

@glensc

glensc commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

Unclear why two jobs failed in CI. No error is printed, just bad exit code.

  set +e
  . $NVM_DIR/nvm.sh && nvm --version
  shell: /usr/bin/bash -e {0}
  env:
    ref: v0.40.0
Error: Process completed with exit code 3.

@ljharb

ljharb commented May 4, 2026

Copy link
Copy Markdown
Member

Those are supposed to fail; you can ignore them.

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will need some tests

Comment thread nvm.sh Outdated
@glensc

glensc commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

Those are supposed to fail; you can ignore them.

image

They make CI red. That's intentional?

Use a more reliable built-in command invocation
@glensc

glensc commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

Seems it's this issue:

Found from nvm-install-test.yml:48:

And long-standing GitHub bug of not having a proper UI for that:

@glensc

glensc commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

urchin tests / tests (sourcing, dash) (pull_request)
urchin tests / tests (sourcing, dash) (pull_request) Failing after 48s
urchin tests / tests (sourcing, sh) (pull_request)
urchin tests / tests (sourcing, sh) (pull_request) Failing after 51s

What to do with these?

✗ Sourcing nvm.sh should ignore a shadowed cd function when auto-detecting NVM_DIR
fake cd error was emitted while sourcing

@glensc glensc requested a review from ljharb May 4, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cdhist: error: unrecognized arguments: -q

2 participants