fix: `reshim` did not rewrite executable path #1311

dylan-chong · 2022-07-25T11:29:01Z

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

alexezio · 2022-07-30T03:04:09Z

hello, @dylan-chong :
What if the execution have more than one shim path?
if we re-generate the reshim file, the shim file will only contains one shim path other than all of their. This will cause issue for origin shim path. could you please run the bats for verify the results?

dylan-chong · 2022-07-30T10:42:04Z

Hey

@alexezio you're right it is breaking some tests.
Why was it implemented as using regex to accumulate data in a file? That gives me the heebie jeebies. Why not take a list of
Also why does the shim have to list the versions of the plugin?

If we could simplify the API such that we remove all shim files and regenerate them all (so asdf reshim takes no arguments) then that would simplify things massively. There would be only one code path:

delete all existing shims
generate shims for all versions of each executable for each plugin

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 ^.
Think it'd be a good improvement to make

alexezio · 2022-07-30T12:40:22Z

hey dylan @dylan-chong :
In my opinion, asdf is stateless, so the shim file have also saved the program version state. If we regenrate them from scrach, We have no where to fetch version state except the file itself.
although regex is annoying, It can modify the version state without break them(by increment).
I am a newbee for asdf, Maybe there is a better approach to reshim. but for now, I vote for regex...
thanks~

dylan-chong · 2022-07-30T21:53:28Z

@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?
Is it not possible to just list all versions of the plugin?

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.

dylan-chong · 2022-07-30T22:02:41Z

Cc @Stratus3D @jthegedus which option would you prefer:

simplify the reshim API and have the command re-generate all files from scratch [for any given plug-in]. Would need a way to be able to list all versions of the plugin supported by the executable
Replace sed with grep so in the existing function we grab out the existing versions and copy them over into a freshly generated file, which is written over the top. This is less error prone since we can use simple grep patterns and generate most of the file from scratch

IMO 1 is better long term , but requires more work now.

dylan-chong · 2022-07-31T00:53:08Z

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 :)

jthegedus · 2022-12-12T08:05:24Z

I am not sure why the tests didn't run here, but merging master so they can. We have some banned command usage which I believe this solution uses, so might need an update.

jthegedus · 2022-12-12T08:30:05Z

lib/commands/reshim.bash

 #!/usr/bin/env bash
-# asdf-plugin: ${plugin_name} ${version}
+`sort -u < "$temp_versions_path"`


We're already using uniq in the codebase, but not the -u flag on sort. Can we change this to be uniq?

so you mean like cat "$temp_versions_path" | sort | uniq?

jthegedus · 2022-12-12T08:31:57Z

@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.

Stratus3D · 2022-12-13T18:50:11Z

lib/commands/reshim.bash

+  temp_dir=${TMPDIR:-/tmp}
+
+  local temp_versions_path
+  temp_versions_path=$(mktemp "$temp_dir/asdf-command-reshim-write-shims.XXXXXX")


I'm not sure line 91-92 are necessary as mktemp should fallback to TMPDIR and /tmp automatically.

@jthegedus, do you have any thoughts since you added this?

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.

lib/commands/reshim.bash

Stratus3D

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.

Stratus3D · 2022-12-13T18:56:38Z

@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.

I completely agree. Let's try to get this shipped. I can re-assess performance later if necessary. Thanks @jthegedus !

Stratus3D

Approving now as I don't want to hold this up. But I think we should address the TMPDIR issue if we can.

jthegedus · 2022-12-20T21:29:54Z

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

dylan-chong requested a review from a team as a code owner July 25, 2022 11:29

dylan-chong changed the title ~~Fix reshim does not rewrite file~~ Fix: reshim does not rewrite file Jul 25, 2022

dylan-chong changed the title ~~Fix: reshim does not rewrite file~~ fix: reshim does not rewrite file Jul 25, 2022

dylan-chong changed the title ~~fix: reshim does not rewrite file~~ fix: reshim did not rewrite file Jul 25, 2022

fix: reshim does not rewrite file

01b4bda

dylan-chong force-pushed the fix-reshim-does-not-rewrite-file branch from 1f5e5c4 to 01b4bda Compare July 25, 2022 11:34

dylan-chong mentioned this pull request Jul 25, 2022

bug: reshim does not update asdf path #1115

Closed

anildigital approved these changes Jul 29, 2022

View reviewed changes

dylan-chong added 3 commits July 31, 2022 12:27

Fix multiple shim versions, but still generate new shim file

ec53493

Remove debugging code, simplify

ca82d92

Don't use banned command

38fe5e3

dylan-chong changed the title ~~fix: reshim did not rewrite file~~ fix: reshim did not rewrite executable path Jul 31, 2022

dylan-chong and others added 4 commits July 31, 2022 12:58

Even less code

f5b293a

Even simpler

dccaa32

Add test

3ce43fd

Merge branch 'master' into fix-reshim-does-not-rewrite-file

91ab906

jthegedus reviewed Dec 12, 2022

View reviewed changes

jthegedus added 4 commits December 12, 2022 19:41

chore: fix SC2006

80bcc40

fix: add tmp_path

c904b07

chore: format code as per shfmt

61226ae

chore: format as per sfmt

96f4722

Stratus3D reviewed Dec 13, 2022

View reviewed changes

lib/commands/reshim.bash Outdated Show resolved Hide resolved

Stratus3D reviewed Dec 13, 2022

View reviewed changes

Delete unused

ea11400

Stratus3D approved these changes Dec 14, 2022

View reviewed changes

jthegedus merged commit 5af7625 into asdf-vm:master Dec 20, 2022

github-actions bot mentioned this pull request Dec 20, 2022

chore(master): release 0.11.1 #1386

Merged

dylan-chong deleted the fix-reshim-does-not-rewrite-file branch December 20, 2022 21:27

hyperupcall pushed a commit to fox-forks/asdf that referenced this pull request Dec 21, 2022

fix: reshim did not rewrite executable path (asdf-vm#1311)

d348ccb

Co-authored-by: James Hegedus <jthegedus@hey.com> Fixes asdf-vm#1115 Fixes asdf-vm#1231 Fixes asdf-vm#1286

This was referenced Dec 23, 2022

bug: lastest version installed from homebrew seems broken #1393

Closed

test: reshim of plugins installed by path #1287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `reshim` did not rewrite executable path #1311

fix: `reshim` did not rewrite executable path #1311

dylan-chong commented Jul 25, 2022 •

edited

Loading

alexezio commented Jul 30, 2022

dylan-chong commented Jul 30, 2022

alexezio commented Jul 30, 2022 •

edited

Loading

dylan-chong commented Jul 30, 2022 •

edited

Loading

dylan-chong commented Jul 30, 2022 •

edited

Loading

dylan-chong commented Jul 31, 2022

jthegedus commented Dec 12, 2022 •

edited

Loading

jthegedus Dec 12, 2022

dylan-chong Dec 14, 2022

jthegedus commented Dec 12, 2022

Stratus3D Dec 13, 2022

dylan-chong Dec 14, 2022

jthegedus Dec 20, 2022

Stratus3D left a comment

Stratus3D commented Dec 13, 2022

Stratus3D left a comment

jthegedus commented Dec 20, 2022

fix: reshim did not rewrite executable path #1311

fix: reshim did not rewrite executable path #1311

Conversation

dylan-chong commented Jul 25, 2022 • edited Loading

Summary

TLDR for comment section

Other Information

alexezio commented Jul 30, 2022

dylan-chong commented Jul 30, 2022

alexezio commented Jul 30, 2022 • edited Loading

dylan-chong commented Jul 30, 2022 • edited Loading

dylan-chong commented Jul 30, 2022 • edited Loading

dylan-chong commented Jul 31, 2022

jthegedus commented Dec 12, 2022 • edited Loading

jthegedus Dec 12, 2022

Choose a reason for hiding this comment

dylan-chong Dec 14, 2022

Choose a reason for hiding this comment

jthegedus commented Dec 12, 2022

Stratus3D Dec 13, 2022

Choose a reason for hiding this comment

dylan-chong Dec 14, 2022

Choose a reason for hiding this comment

jthegedus Dec 20, 2022

Choose a reason for hiding this comment

Stratus3D left a comment

Choose a reason for hiding this comment

Stratus3D commented Dec 13, 2022

Stratus3D left a comment

Choose a reason for hiding this comment

jthegedus commented Dec 20, 2022

fix: `reshim` did not rewrite executable path #1311

fix: `reshim` did not rewrite executable path #1311

dylan-chong commented Jul 25, 2022 •

edited

Loading

alexezio commented Jul 30, 2022 •

edited

Loading

dylan-chong commented Jul 30, 2022 •

edited

Loading

dylan-chong commented Jul 30, 2022 •

edited

Loading

jthegedus commented Dec 12, 2022 •

edited

Loading