-
Notifications
You must be signed in to change notification settings - Fork 774
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
fix: reshim
did not rewrite executable path
#1311
fix: reshim
did not rewrite executable path
#1311
Conversation
reshim
does not rewrite file
reshim
does not rewrite filereshim
does not rewrite file
reshim
does not rewrite filereshim
did not rewrite file
1f5e5c4
to
01b4bda
Compare
hello, @dylan-chong : |
Hey @alexezio you're right it is breaking some tests. If we could simplify the API such that we remove all shim files and regenerate them all (so
There's no need to keep files around and do complicated regexes on them if we can regenerate them from scratch every time (which is what you have to do when upgrading asdf with homebrew anyway). Unless, there's something I'm missing? Happy to have a go at implementing this idea if you'd like ^. |
hey dylan @dylan-chong : |
@alexezio but you can delete the shim files manually and regenerate the files from scratch? Does this not put the version comments in the file? if you would prefer to keep the regex I could have a go at replacing everything except the existing version list using the regex. This would fix the issue in the description. |
Cc @Stratus3D @jthegedus which option would you prefer:
IMO 1 is better long term , but requires more work now. |
The easiest option was to implement no 2 (see above), which I have committed to this branch. @Stratus3D @jthegedus This is ready to merge @alexezio This means that the change to the sed pattern in your PR #1287 is not longer required. But it would still be great to merge the test that you wrote in your PR. I have checked and that test passes in this branch, but I don't want to steal your contribution so I'll let you commit that :) |
reshim
did not rewrite filereshim
did not rewrite executable path
I am not sure why the tests didn't run here, but merging |
lib/commands/reshim.bash
Outdated
#!/usr/bin/env bash | ||
# asdf-plugin: ${plugin_name} ${version} | ||
`sort -u < "$temp_versions_path"` |
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.
We're already using uniq
in the codebase, but not the -u
flag on sort
. Can we change this to be uniq
?
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.
so you mean like cat "$temp_versions_path" | sort | uniq
?
@Stratus3D looking at our open PRs we have 3 which attempt to resolve this issue. Are you able to check this as it seems to me to be the best option. I am concerned about performance, but here correctness is preferred, we can come back and fix perf. |
temp_dir=${TMPDIR:-/tmp} | ||
|
||
local temp_versions_path | ||
temp_versions_path=$(mktemp "$temp_dir/asdf-command-reshim-write-shims.XXXXXX") |
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'm not sure line 91-92 are necessary as mktemp
should fallback to TMPDIR
and /tmp
automatically.
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.
@jthegedus, do you have any thoughts since you added this?
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.
This seemed fine to me, but after looking at existing use of mktemp
this was the outlier, so I changed it to align with the other usages. We can back this out later once we clean up and decide to put this behind a function or something.
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.
Other than my two comments above this PR looks good to me and I think we should get it merged.
I agree with @dylan-chong that approach #1 is better but this PR should work for now.
I completely agree. Let's try to get this shipped. I can re-assess performance later if necessary. Thanks @jthegedus ! |
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.
Approving now as I don't want to hold this up. But I think we should address the TMPDIR
issue if we can.
Let's see how it goes 😀 |
Co-authored-by: James Hegedus <jthegedus@hey.com> Fixes asdf-vm#1115 Fixes asdf-vm#1231 Fixes asdf-vm#1286
Summary
Fixes reshimming not working by always recreating the file from scratch.
We grab the existing plugin versions from the existing shim file and squish those into a brand new shim file.
This solves the problem of updating asdf from homebrew causing the executable path to be wrong, and not fixable by
asdf reshim
Regenerating the shim file from scratch is much simpler, no additional tests required like #1287.
Fixes: #1115
Fixes: #1231 (dupe)
Fixes: #1286
Makes change in #1287 not needed, although the test is still useful
TLDR for comment section
Read from this comment down #1311 (comment)
Other Information