-
Notifications
You must be signed in to change notification settings - Fork 874
feat(golang-rewrite): add support for shim templates resolution #2067
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.
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
feat(golang-rewrite): add support for shim templates resolution #2067
Conversation
Hi @augustobmoura . I'm not actually sure custom shim templates are needed for Elixir. The plugin seems to work fine for me without the custom shim for Was I wrong in my understanding when I removed this feature? See https://asdf-vm.com/guide/upgrading-to-v0-16.html#custom-shim-templates-are-no-longer-supported |
Shim templates are still being used by asdf-nodejs and asdf-python for auto-reshimming after running global installs. We could think of a different mechanism for this, instead of shim templates. We need a way of intercepting calls to commands and conditionally marking them for auto-reshiming after the command is finished. Similar to plugin hooks, but defined at the plugin level, instead of user level. Maybe we could implement a way of having plugin hooks at mutiple levels? plugin, user, and maybe system level for example. And all of them are called when the hook is triggered, wdyt? |
@augustobmoura thanks for the clarification. I guess I'd be inclined to add back in support for custom shim templates. I'm not a fan of how asdf hooks are designed. And hooks are not controlled by the plugin, they are managed by the user. I'm going to work to get this PR merged soon. |
shimsDir, err := os.Stat(path.Join(plugin.Dir, "shims")) | ||
if err == nil && shimsDir.IsDir() { | ||
paths = append(paths, path.Join(plugin.Dir, "shims")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these 4 lines necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old bash implementation added plugin/shims
to the PATH
environment variable, I don't think the code in asdf-nodejs will stop working if we omit it, but I wanted to keep it the same as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this code back into #2076 which addresses the linter warnings present here. Closing this PR in favor of that one (it still contains all your commits).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think the changes here are great. My only concern is that I don't think shim template code belongs in the ExecutablePaths
function as that is only suppose to be the executable paths returned by the list-bin-paths
plugin callback.
Summary
In this PR I'm adding support for custom shim templates back, since it is missing from the current go implementation.
As of now, the implementation short-circuits very early if it finds an acceptable shim in the first tool available.
Plugins that I know are using shim templates are: asdf-elixir, asdf-nodejs, and asdf-python.
fixes #2025 asdf-vm/asdf-nodejs#421