Skip to content

Fix tac on OSX if coreutils are linked in path#216

Merged
Stratus3D merged 2 commits intoasdf-vm:masterfrom
jrogov:fix/osx-tac-coreutils-in-path
Mar 24, 2021
Merged

Fix tac on OSX if coreutils are linked in path#216
Stratus3D merged 2 commits intoasdf-vm:masterfrom
jrogov:fix/osx-tac-coreutils-in-path

Conversation

@jrogov
Copy link
Copy Markdown
Contributor

@jrogov jrogov commented Mar 19, 2021

One of recent commits (6667b31) dealed with tac not being present on PATH on OSX systems.
The problem is that coreutils is widely installed (via brew) on OSX systems by developers and it in fact includes missing tac utility! But its tail is missing -r flag:

~/c/asdf-nodejs (fix/osx-tac-coreutils-in-path) $ asdf list-all nodejs
tail: invalid option -- 'r'
Try 'tail --help' for more information.

This PR adds a check before implementing tac over tail to see if tac is present.

@Stratus3D
Copy link
Copy Markdown
Member

Stratus3D commented Mar 19, 2021

Thanks for the PR! If tail -r isn't available on all platforms where tac is missing I think we should fix it instead of changing the conditional. Is there an implementation of tac in Bash that will work on all platforms?

@Stratus3D
Copy link
Copy Markdown
Member

Stratus3D commented Mar 19, 2021

What about this for the tac implementation (taken from https://unix.stackexchange.com/a/9358/74959)?

cat -n | sort -nrk1 | cut -f2-

Can you test this out on your machine?

And, if what are you thoughts on changing the conditional to this?

if ! command -v tac &>/dev/null; then

@jrogov
Copy link
Copy Markdown
Contributor Author

jrogov commented Mar 22, 2021

You're right, it's better to make it fallback for everyone.
The solution you've proposed works on macOS Big Sur with default binaries.

/tmp $ echo A>a
/tmp $ echo B>b
/tmp $ echo C>c

/tmp $   tac() {
    /bin/cat -n "$@" | /usr/bin/sort -nrk1 | /usr/bin/cut -f2-
  }
/tmp $ tac a b c
C
B
A

Made the changes, anything else?

On a side note: caching for this plugin is heavily needed.

@Stratus3D
Copy link
Copy Markdown
Member

Thanks for updating this @arikai ! I was on vacation, sorry for the late merge!

@Stratus3D Stratus3D merged commit 39fd2d0 into asdf-vm:master Mar 24, 2021
@jrogov
Copy link
Copy Markdown
Contributor Author

jrogov commented Mar 24, 2021

Don't worry, just was trying to understand the circumstances :)
Hope you had a nice one!

seivan added a commit to seivan/asdf-nodejs that referenced this pull request Mar 26, 2021
seivan added a commit to seivan/asdf-nodejs that referenced this pull request Apr 1, 2021
augustobmoura pushed a commit that referenced this pull request Apr 9, 2021
* Fix tac on osx if coreutils are linked in path
* Change fallback condition for `tac`. Provide space-safe implementation
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.

2 participants