Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prefer readlink to greadlink #1484

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Prefer readlink to greadlink #1484

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion libexec/rbenv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export RBENV_DIR

canonicalize() {
local readlink resolved_path
if readlink="$(type -P greadlink)" || readlink="$(type -P readlink)"; then
if readlink="$(type -P readlink)" || readlink="$(type -P greadlink)"; then
Copy link
Member

@mislav mislav Feb 20, 2023 •
edited

Choose a reason for hiding this comment

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

We mostly check greadlink because of older macOS versions (without readlink -f) where Homebrew or Macports might have placed a GNU readlink into PATH. Checking first readlink then greadlink nulifies that use-case because readlink is always available on macOS (but may or may not support -f).

You're right that our fallback to manual resolution mostly alleviates the need for greadlink, but I felt that if greadlink is present, let's just use it. I didn't think there would be significant overhead with PATH lookups for anyone.

In fact, applying your change does not net any tangible improvements on my machine.

# with this PR
$ hyperfine --shell=none --warmup 3 'bin/rbenv --version'
Benchmark 1: bin/rbenv --version
  Time (mean ± σ):       6.7 ms ±   0.8 ms    [User: 1.3 ms, System: 3.6 ms]
  Range (min … max):     5.6 ms …  11.8 ms    296 runs

# without this PR - around 0.5 ms slower
$ hyperfine --shell=none --warmup 3 'bin/rbenv --version'
Benchmark 1: bin/rbenv --version
  Time (mean ± σ):       7.2 ms ±   0.7 ms    [User: 1.3 ms, System: 3.9 ms]
  Range (min … max):     6.1 ms …  11.7 ms    280 runs

I would say that this change benefits only people with very slow PATH resolution, like on your machine, and that the proper fix would be to change your PATH settings (or your resolver) to be faster.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your comment!

the proper fix would be to change your PATH settings (or your resolver) to be faster.

As for #1483, it is possible. I can put the Linux directory before the Windows directory in the PATH.

However, it is difficult to work around this issue. WSL2 inserts many NTFS directories in the PATH by default, and they are actually needed to be able to transparently launch Windows executables from WSL2 environment.

One possible hack is to put /usr/local/bin/greadlink as a symbolic link to /usr/bin/readlink. TBH, I don't want to do that on all my Windows machines.

Your experiment shows an improvement of 0.5 ms on macOS. I admit that it is not so big, but I am seeing the same improvement on my macOS. I believe this change has little maintenance concern. I would appreciate your positive consideration.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to the change. However, please wait a few weeks for me to reach a decision, as I am just leaving to vacation. 🙇

# happy path: GNU & BSD readlink, macOS 12.3+
if resolved_path="$("$readlink" -f "$1" 2>/dev/null)"; then
printf "%s\n" "$resolved_path"
Expand Down