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

bug: Can not get a list of installed versions from a plugin. #1372

Closed
kennyp opened this issue Nov 18, 2022 · 9 comments · Fixed by #1424
Closed

bug: Can not get a list of installed versions from a plugin. #1372

kennyp opened this issue Nov 18, 2022 · 9 comments · Fixed by #1424
Labels

Comments

@kennyp
Copy link
Contributor

kennyp commented Nov 18, 2022

Describe the Bug

An issue posted on the golang plugin brought to my attention that head breaks the plugin if legacy_version_file is enabled. Because of the semantics of how the go.mod file is supposed to be interpreted, parse-legacy-file depends on the ability to get a list of the currently installed versions. What is a safe way to get a list of installed versions without calling back into asdf list and breaking things?

Steps to Reproduce

  1. asdf update --head
  2. echo 'legacy_version_file = yes' >> ~/.asdfrc
  3. asdf plugin-install golang
  4. mkdir example; cd example; go mod init example.com/m
  5. asdf list golang

Expected Behaviour

Nothing hangs.

Actual Behaviour

Infinite recursion resulting in hanging and possible crashing of the shell/terminal.

Environment

OS:
Linux kparnell-ltl 5.15.0-52-generic #58~20.04.1-Ubuntu SMP Thu Oct 13 13:09:46 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

SHELL:
GNU bash, version 5.0.17(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

ASDF VERSION:
v0.10.2-24d524c

ASDF ENVIRONMENT VARIABLES:
ASDF_DIRENV_BIN=/home/kparnell/.asdf/installs/direnv/2.32.1/bin/direnv
ASDF_DIR=/home/kparnell/.asdf

ASDF INSTALLED PLUGINS:
direnv                       [email protected]:asdf-community/asdf-direnv.git master 1b93f6e
elixir                       [email protected]:asdf-vm/asdf-elixir.git master d08f506
erlang                       [email protected]:asdf-vm/asdf-erlang.git master 0463971
golang                       [email protected]:kennyp/asdf-golang.git master cc8bc47
java                         [email protected]:halcyon/asdf-java.git master b76a5e3
jq                           [email protected]:azmcode/asdf-jq.git master af8cad8
kotlin                       [email protected]:asdf-community/asdf-kotlin.git master af9ab2c
lua                          [email protected]:Stratus3D/asdf-lua.git master d99f76c
nim                          [email protected]:asdf-community/asdf-nim main e305768
nodejs                       [email protected]:asdf-vm/asdf-nodejs.git master c9e5df4
python                       [email protected]:danhper/asdf-python.git master 8505457
ruby                         [email protected]:asdf-vm/asdf-ruby.git master 4ff69fe
rust                         [email protected]:code-lever/asdf-rust.git master 0c88f99
shfmt                        [email protected]:luizm/asdf-shfmt.git master a42c5ff
terraform                    [email protected]:Banno/asdf-hashicorp.git master 3122c04
yq                           [email protected]:sudermanjr/asdf-yq.git master 772992f


### asdf plugins affected (if relevant)

github.com:kennyp/asdf-golang.gi
@kennyp kennyp added the bug label Nov 18, 2022
@Stratus3D
Copy link
Member

@kennyp sorry for the late reply here. Does this affect any other plugins? If not I suspect the golang plugin is doing something odd.

@Stratus3D
Copy link
Member

As a general rule plugins should not invoke asdf, otherwise you may create cyclic dependency that results in infinite recursion. I suspect asdf list is invoking your parse-legacy-file callback, which in turn invokes asdf list, and whole cycle repeats over and over. Adding a few simple prints to your parse-legacy-file callback should validate this theory.

The line I am referring to is https://github.com/kennyp/asdf-golang/blob/master/bin/parse-legacy-file#L19. It should be possible to refactor the callback so that line isn't necessary.

@kennyp
Copy link
Contributor Author

kennyp commented Dec 16, 2022

@Stratus3D that all makes sense, but given how the legacy file is supposed to be interpreted, how do you recommend I get a list of installed versions without using using asdf list?

@Stratus3D
Copy link
Member

Am I correct in summarizing the process as this?

  • Read the legacy version file to get the major and minor Go versions
  • List all installed Go versions with asdf list. Pick an installed Go version matching the major and minor versions from the legacy file
  • Return the full major.minor.patch version

@kennyp
Copy link
Contributor Author

kennyp commented Dec 16, 2022

@Stratus3D close. We really only care about the major version. So we return the latest installed version that matches.

@kennyp
Copy link
Contributor Author

kennyp commented Dec 17, 2022

@Stratus3D fixed the issue in asdf-golang/93 by taking some inspiration from utils.bash, but that seems like it could be brittle in the future. Any help is appreciated.

@Stratus3D
Copy link
Member

Looking at the docs I see we don't document this, but the bin/parse-legacy-file callback script should be idempotent. Given the same legacy version file, it should return the same exact version regardless of the state of the machine on which it is run. If bin/parse-legacy-file looked at installed versions to compute the full version, two users could end up with two different versions for the same project. Breaking asdf's deterministic guarantees when it comes to versions. So I don't think this is something we want to support at all. bin/parse-legacy-file should return the same version regardless of what the user has installed.

Remember, there are different golang versions because there are different revisions of the code, and those revisions were made to change golang behavior. So if those users end up using two different versions, there is always the chance they will observe different behavior, and that's something we want to avoid with asdf. See #1012 for a related discussion.

I know this probably isn't what you want, but I do think this is the right choice for asdf core. You have a couple options:

  • It might be better to just require users to define the full Go version in a .tool-versions file for each project. Just because a legacy version file exists doesn't mean the version data in it is suitable for asdf. (best option).
  • Simplify your bin/parse-legacy-file script to only return the version as it is defined in go.mod, and update your bin/install script to accept those incomplete versions from go.mod, and then define code in your install script that resolves that incomplete version to the latest matching full version (and disregard what is installed on the machine). This will ensure that two users who run asdf install for the same project at the exact same time will get the exact same version. But if they run asdf install at different times they might get different versions (so it's not truly deterministic). This might be a good middle ground.
  • You are free to keep the workaround you've implemented (though I wouldn't recommend it).

I will propose an update to the docs.

@Stratus3D
Copy link
Member

@jthegedus what do you think about me changing this:

bin/parse-legacy-file

This can be used to further parse the legacy file found by asdf. If parse-legacy-file isn't implemented, asdf will simply cat the file to determine the version. The script will be passed the file path as its first argument.

To this:

bin/parse-legacy-file

This can be used to further parse the legacy file found by asdf. If parse-legacy-file isn't implemented, asdf will simply cat the file to determine the version. The script will be passed the file path as its first argument. Note that this script should be deterministic and always return the same exact version when parsing a legacy file. The script should return the same version regardless of what is installed on the machine or whether the version of the tool version is valid or complete. Some legacy file formats may not be suitable.

@jthegedus
Copy link
Contributor

@Stratus3D That change makes sense.

As someone who works with Go, I am often confused by this practice of using major versions only. Minor and patch versions exist because they're different, so shouldn't be ignored by teams writing Go or else will introduce inconsistencies. I have given up on this "crusade" at my workplace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants