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.

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

Always rewrite shims, not just update meta-data. #893

Closed
wants to merge 1 commit into from

Conversation

seivan
Copy link

@seivan seivan commented Mar 15, 2021

Summary

Always rewrite shims from scratch instead of doing meta-data comparison and updates, this means less state to maintain and less avenues where things can go wrong.
This also allows Homebrew to setup a post_install that triggers asdf reshim.

Details

So as I understand this, this checks if the meta-data inside a shim matches the expected version and plugin and rewrites the meta-data if it doesn't.

sed -i.bak -e "s/exec /# asdf-plugin: ${plugin_name} ${version}\\"$'\n''exec /' "$shim_path"
rm "$shim_path".bak

One of the issues is, that it doesn't check to see if the path to asdf has changed, this can happen when homebrew updates and asdf finds itself changed from ..../0.8.o/asdf to ..../0.8.1/asdf
But we never check the path, just the meta-data.

exec /usr/local/Cellar/asdf/0.8.0/libexec/bin/asdf exec "iex" "$@"

If a new version is released in Homebrew, the path above will be invalid.

I am open to adding a different command or an option like --force, but in general I think this is for the better unless I've missed a substantial reason for doing meta-data comparison instead of just rewriting the file each time.
Performance is not it, as it is equally fast.

Edit: Might want to point out that this isn't 100% necessary if we want to skip asdf path being under a versioned namespace.

We can always just resort to exec /usr/local/opt/asdf/libexec/bin/asdf exec "iex" "$@" but for some reason I think that having asdf namespaced under a version to be good hygiene, but it will require a reshim on asdf updates. This can be automated with post_install hook but needs changes from this PR.

Related:
Homebrew/homebrew-core#73173
#891

Always rewrite shims from scratch instead of doing meta-data comparison and updates, this means less state to maintain and less avenues where things can go wrong.
@seivan seivan requested a review from a team as a code owner March 15, 2021 16:36
seivan added a commit to seivan/homebrew-core that referenced this pull request Mar 15, 2021
Switch to `opt_libexec` which translates to `opt_prefix/"libexec`, this means that there will be no migration steps for shims. 
However till adding a `$ asdf reshim` `post_install` hook incase there is an update that needs to be done on the shims when `asdf` itself updates to a new version or formula revision. There are changes pending on  [`asdf-vm`](asdf-vm/asdf#893) that would allow regenerating the shims in case they need to be.
@seivan seivan closed this Mar 16, 2021
@jthegedus
Copy link
Contributor

Can you describe why this is closed? Is it merely because the Homebrew changes no longer require this?

@seivan
Copy link
Author

seivan commented Mar 16, 2021

Can you describe why this is closed? Is it merely because the Homebrew changes no longer require this?

Correct, I would like actually like this, but I can't be c.b.a right now.

It essentially means that shims would be tied to a version of asdf and each new distribution of asdf (either update or revision change) would require a reshim.
But I changed the Homebrew PR to be linked to current version of asdf instead of a specific version, rendering this change unnecessary.

Edit: Could pick this up in the future, but for now my focus is on fixing broken asdf installation and plugins, so I can remove my forks.

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.

None yet

2 participants