fix: asdf latest --all report wrong runtimes (#1180) #1191

roele · 2022-03-30T23:52:58Z

Summary

Revisites the logic for command latest --all which returns wrong version numbers if the plugin uses multiple distros/variants (e.g. Java, Python, Ruby, ...).

Instead of falling back to use the latest available version (tail -1) from all the available versions via list all, the code tries to list specific versions for installed distros. This is done by getting a distinct list of installed distros assuming the format - and then getting the latest version for that specific distro via additional version query. If a plugin has no installed versions, the old behaviour showing the latest from all available versions is used.

Additionally if multiple distros are installed (e.g. Java; opendjdk, zulu) the output lists each installed one with its latest version and status.

  ❯ asdf list
direnv
  2.31.0
golang
  1.17
  1.18
java
  openjdk-11.0.2
  openjdk-17.0.2
  openjdk-18
  zulu-17.32.13
nodejs
  16.14.2
  lts
ruby
  No versions installed

  ❯ asdf latest --all
direnv              2.31.0                        installed
golang              1.18                          installed
java                openjdk-18                    installed
java                zulu-18.28.13                 missing
nodejs              lts                           installed
ruby                truffleruby+graalvm-22.0.0.2  missing

Fixes: #1180

Other Information

lib/commands/command-latest.bash

Stratus3D · 2022-06-08T20:11:37Z

Thanks for the PR and sorry for the late review here @roele. Would implementing the correct logic in a plugin's latest-stable callback fix this issue for plugins that have multiple distros/variants of the tool? This doesn't seem like something that should be handled by asdf core.

roele · 2022-06-09T16:20:51Z

@Stratus3D Unfortunately I'm not familiar with this callback. As far as i have seen none of the plugins implement latest-stable. Isn't this callback supposed to return the latest stable version for a given query string. I don't see how this could help when someone has installations of multiple vendors (e.g. Java OpenJDK, Corretto, Microsoft). The callback would need to return the latest version for each of them and then the asdf core would still need to deal with handling those (comparing them against installed versions).

jthegedus · 2022-07-17T09:27:23Z

@Stratus3D Your proposal would break asdf install <tool> latest usage, in that it should return only 1 version. I expect the <plugin>/bin/latest-stable script to return a single version, even for a plugin like java which supports many versions. It is difficult to rely on latest-stable when plugins can support many distributions. Adding query string support to latest-stable may have been a mistake because we now tend to use it in places where we desire a query string filter but cannot know the appropriate filter, so have to fallback to <plugin>/bin/list-all.

I think the solution here is the most appropriate.

asdf latest --all may have been a bad command to introduce as it is purely informational, difficult to implement and benefit not particularly significant or even clear.

jthegedus · 2022-07-17T09:31:49Z

test/fixtures/dummy_distro_plugin/bin/list-all

@@ -0,0 +1,6 @@
+#!/usr/bin/env bash
+
+versions_list=(acme-1.0.0 acme-1.1.0 distro-2.0.0)


We should also test this codepath for versions which have

multiple prefix parts, EG: zulu-jre-musl-8.42.0.23

suffixes after the semver version

where list-all returns non-prefixed/suffixed versions.

asdf python shows examples of all these types:

... 3.10.5 3.11.0b4 3.11-dev 3.12-dev activepython-2.7.14 activepython-3.5.4 activepython-3.6.0 anaconda-1.4.0 anaconda-1.5.0 ... pypy3.6-7.3.3 pypy3.7-c-jit-latest pypy3.7-7.3.2-src ... pypy3.9-7.3.8 pypy3.9-7.3.9-src pypy3.9-7.3.9 pyston-2.2 pyston-2.3 pyston-2.3.1 pyston-2.3.2 pyston-2.3.3 pyston-2.3.4 stackless-dev stackless-2.7-dev

pypy3.9-7.3.8 pypy3.9-7.3.9-src

IMO these are two different variants, though most querying added to commands are prefix-only queries, so I guess these should be resolved as a single variant.

Good point, current implemenation does not yet handle suffixes. But i agree that this example should be treatet as a single variant. I would consider suffixes after the sematic version not as a variant but rather a special version like beta, GA, src or similar.

jthegedus · 2022-07-17T09:42:35Z

lib/functions/versions.bash

+        for variant in $installed_variants; do
+          local version
+          version=$(list_all_command "$plugin_name" "$(echo "$variant" | tr -d '[:space:]')" |
+            grep -ivE "(^Available version:|-src|-dev|-latest|-stm|[-\\.]rc|-alpha|-beta|[-\\.]pre|-next|(a|b|c)[0-9]+|snapshot|master)" |
+            # filter for exact match or with version suffix to deal with overlaps (e.g. zulu-18.28.13, zulu-jre-18.28.13)
+            grep -iE "^($variant|$variant(-\d+.*)?)\$" |
+            sed 's/^[[:space:]]\+//' |
+            tail -1)
+


While shorter to write this way, we're calling list_all_command for each variant when really it only needs to be called once for the plugin. The list_all_command executes <plugin>/bin/list-all which performs a network call and file read/writes. It is usually a really slow command. Can you rework this so list_all_command is called once per plugin and not variant?

Very valid concern, will rework this part 👍

* address PR comments

Stratus3D · 2022-07-25T16:24:14Z

lib/functions/versions.bash

+        sed -e "s/\(^.*\)\-[0-9].*/\1/" |   # extract variant prefix
+        sort -u |                           # unique
+        sed -e "s/^[[:space:]]*[0-9].*//" | # remove versions without variant prefix
+        sed '/^[[:space:]]*$/d')            # remove empty lines


I'm uncomfortable with this being part of asdf. I know we have similar logic that already exists in a few places, but this doesn't feel like a future-proof solution. We're effectively using versions to store more than the version number. Versions are a combination of tool type (e.g. openjdk) and version (e.g. 1.2.3). I know this is a pretty fundamental change, but I think we might be better off either introducing a type field in addition to version, or specifying that plugins only support one type of thing (e.g. instead of a java plugin we have an openjdk plugin). Thoughts?

specifying that plugins only support one type of thing

interesting proposal. I know this would also upset the Hashicorp plugin which manages the binaries for the Hashi ecosystem and not just different sources of the same runtime. I feel like Java and Hashicorp plugins are different enough and both valid in the spec we have defined 🤔

jthegedus · 2022-08-30T15:37:17Z

As I stated earlier

asdf latest --all may have been a bad command to introduce as it is purely informational, difficult to implement and benefit not particularly significant or even clear.

After further thought, I think the most beneficial thing to do is remove this --all flag from the latest command.

asdf list could return the latest version available vs what's installed locally #1036 was initial latest --all discussion
feat(latest): adds the flag --all to the latest command #1096 was initial PR
fix: latest --all correctly report plugins as missing #1118 fixed a bug in initial implementation
bug: asdf latest --all report wrong runtimes #1180 is another discussion of how this is not right
fix: asdf latest --all report wrong runtimes (#1180) #1191 (this PR) is a fix for bug: asdf latest --all report wrong runtimes #1180

The Issues discussing the desire for the command and the current obtuse behaviour have a total of 3 participants (outside asdf core team) and seemingly no support for this behaviour (use Issue/PR GitHub emojis as the metric).

I propose we remove it.

@Stratus3D Do you have any strong feelings either way?

@roele I'm sure hearing this is unpleasant given your input on this PR was significant. I am sorry. Are there points I am missing which might convince me otherwise?

roele · 2022-08-30T18:50:53Z

@jthegedus Even though i would love the convenience of a single command telling me what packages are outdated, I am fine with that decision and understand that maintainability has upmost priority.

jthegedus · 2022-08-31T09:55:26Z

@jthegedus Even though i would love the convenience of a single command telling me what packages are outdated, I am fine with that decision and understand that maintainability has upmost priority.

I agree on the convenience. Perhaps this is a good candidate for a one-line shell cmd to demonstrate in a cookbook/recipes section in the docs.

Until the points raised above by @Stratus3D are addressed I don't think this passes any cost/benefit analysis.

I will close this PR. I will raise a PR to back out the latest --all feature.

Thanks for your contributions here and I hope you contribute again 🙏

roele requested a review from a team as a code owner March 30, 2022 23:52

jvcastaneda approved these changes Apr 4, 2022

View reviewed changes

roele commented Apr 4, 2022

View reviewed changes

lib/commands/command-latest.bash Outdated Show resolved Hide resolved

jthegedus reviewed Jul 17, 2022

View reviewed changes

Roland Schaer and others added 2 commits July 23, 2022 16:20

fix: asdf latest --all report wrong runtimes (#1180)

d177f87

fix: asdf latest --all report wrong runtimes (#1180)

489548f

* address PR comments

Stratus3D reviewed Jul 25, 2022

View reviewed changes

jthegedus closed this Aug 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: asdf latest --all report wrong runtimes (#1180) #1191

fix: asdf latest --all report wrong runtimes (#1180) #1191

roele commented Mar 30, 2022 •

edited

Stratus3D commented Jun 8, 2022

roele commented Jun 9, 2022

jthegedus commented Jul 17, 2022 •

edited

jthegedus Jul 17, 2022 •

edited

jthegedus Jul 17, 2022

roele Jul 23, 2022

jthegedus Jul 17, 2022 •

edited

roele Jul 23, 2022 •

edited

Stratus3D Jul 25, 2022

jthegedus Jul 27, 2022 •

edited

jthegedus commented Aug 30, 2022

roele commented Aug 30, 2022

jthegedus commented Aug 31, 2022

		@@ -0,0 +1,6 @@
		#!/usr/bin/env bash

		versions_list=(acme-1.0.0 acme-1.1.0 distro-2.0.0)

fix: asdf latest --all report wrong runtimes (#1180) #1191

fix: asdf latest --all report wrong runtimes (#1180) #1191

Conversation

roele commented Mar 30, 2022 • edited

Summary

Other Information

Stratus3D commented Jun 8, 2022

roele commented Jun 9, 2022

jthegedus commented Jul 17, 2022 • edited

jthegedus Jul 17, 2022 • edited

Choose a reason for hiding this comment

jthegedus Jul 17, 2022

Choose a reason for hiding this comment

roele Jul 23, 2022

Choose a reason for hiding this comment

jthegedus Jul 17, 2022 • edited

Choose a reason for hiding this comment

roele Jul 23, 2022 • edited

Choose a reason for hiding this comment

Stratus3D Jul 25, 2022

Choose a reason for hiding this comment

jthegedus Jul 27, 2022 • edited

Choose a reason for hiding this comment

jthegedus commented Aug 30, 2022

roele commented Aug 30, 2022

jthegedus commented Aug 31, 2022

roele commented Mar 30, 2022 •

edited

jthegedus commented Jul 17, 2022 •

edited

jthegedus Jul 17, 2022 •

edited

jthegedus Jul 17, 2022 •

edited

roele Jul 23, 2022 •

edited

jthegedus Jul 27, 2022 •

edited