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

feat: mark current resolved versions in asdf list output #762

Merged
merged 6 commits into from
Jun 21, 2022

Conversation

botp
Copy link
Contributor

@botp botp commented Jul 12, 2020

Summary

updated code to pass thru basic workflow checks

Closes #617

@botp botp requested a review from a team as a code owner July 12, 2020 11:06
@Stratus3D
Copy link
Member

Stratus3D commented Jul 16, 2020

Thanks for the PR! Can you explain what these changes do? And do you have an issue in the issue tracker for this bug/feature?

Also, it looks like the format check failed.

@botp
Copy link
Contributor Author

botp commented Jul 17, 2020 via email

@Stratus3D
Copy link
Member

This seems similar to #617

@botp
Copy link
Contributor Author

botp commented Jul 23, 2020 via email

@Stratus3D Stratus3D changed the title updated to pass wflow checks eg thru lint etc Flag installed versions in asdf list output Jun 13, 2022
@jthegedus jthegedus changed the title Flag installed versions in asdf list output feat: flag installed versions in asdf list output Jun 17, 2022
@jthegedus
Copy link
Contributor

Just a note, this flag will likely break people who are parsing this as white-space delimited.

@botp
Copy link
Contributor Author

botp commented Jun 18, 2022 via email

@jthegedus
Copy link
Contributor

jthegedus commented Jun 20, 2022

On Fri, Jun 17, 2022 at 3:53 PM James Hegedus @.***> wrote: Just a note, this flag will likely break people who are parsing this as white-space delimited.
maybe we may put the flag after the version number so it may not intrude w existing scripts/parsers? anyway, once we finalize on the format, the rest would be easy.

Sorry, I was incorrect in this. I thought we were prefixing with either [:space:][:space:]<version> or *[:space:]<version>. With [:space:]*<version> it will be fine.

I will rebase and merge as there doesn't seem to be any objection to this feature, thanks.

@jthegedus jthegedus changed the title feat: flag installed versions in asdf list output feat: mark current resolved versions in asdf list output Jun 20, 2022
@jthegedus
Copy link
Contributor

jthegedus commented Jun 20, 2022

Here is the output from the rebased code:

.asdf on  master [!] 
➜ pwd
/home/jthegedus/.asdf

.asdf on  master [!] 
➜ asdf list
deno
 *1.22.1
firebase
  10.9.2
  11.1.0
java
  openjdk-18.0.1.1
nodejs
  16.14.2
  16.15.1
pnpm
  6.13.0
  7.2.1
shellcheck
  0.8.0
shfmt
  3.5.1
v
 *weekly.2022.20
  weekly.2022.21

.asdf on  master [!] took 163ms 
➜ cat ~/.tool-versions
───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /home/jthegedus/.tool-versions
───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ v weekly.2022.20
   2   │ deno 1.22.1
───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────

Notably, it doesn't list if a version is missing, and I'm not sure if we want the output to do that or not.

For instance, I am in my system ~/.asdf repo, which technically has a version of Shellcheck & Shell Format selected for use, but not currently installed:

deno            1.22.1          /home/jthegedus/.tool-versions
firebase        ______          No version is set. Run "asdf <global|shell|local> firebase <version>"
java            ______          No version is set. Run "asdf <global|shell|local> java <version>"
nodejs          ______          No version is set. Run "asdf <global|shell|local> nodejs <version>"
pnpm            ______          No version is set. Run "asdf <global|shell|local> pnpm <version>"
shellcheck      0.7.2           Not installed. Run "asdf install shellcheck 0.7.2"
shfmt           3.3.0           Not installed. Run "asdf install shfmt 3.3.0"
v               weekly.2022.20  /home/jthegedus/.tool-versions

Any thoughts on this, or do we agree that asdf list should only show versions installed on the system, and then this just matches those installed versions that are then resolved/selected by asdf?

@jthegedus
Copy link
Contributor

jthegedus commented Jun 20, 2022

Interestingly the test suites on Ubuntu are failing on installing Fish or something🤷
I have re-run the Action a number of times to see if it was just an intermittent issue. It is not. So I will investigate this soon.

EDIT: it is the Elvish binary download URL failing. Seems something is going on with their setup.

@jthegedus jthegedus merged commit 5ea6795 into asdf-vm:master Jun 21, 2022
@jthegedus
Copy link
Contributor

jthegedus commented Jun 21, 2022

Merged due to elvish failures being an external issue that will eventually be resolved. Ran tests locally without issue.

EDIT: I reported the Elvish issue and it has since been resolved. I re-ran the failed jobs on the latest commit on this PR and they succeeded.

@Stratus3D
Copy link
Member

@jthegedus why did you select this PR over #1192? They are almost identical PRs. Unless there is something we want from #1192 it can be closed.

parsing this as white-space delimited.
maybe we may put the flag after the version number so it may not intrude w existing scripts/parsers? anyway, once we > finalize on the format, the rest would be easy.

Sorry, I was incorrect in this. I thought we were prefixing with either [:space:][:space:] or [:space:]. > With [:space:] it will be fine.

I think regardless of formatting this will break existing parsers. I suggest with stick with whatever formatting will be the most useful going forward (we're already going to have to bump to at least 0.11.0 because this feature has been merged). [:space:]*<version> may not be best because the star and version would be considered the same field by commands like cut. *[:space:]<version> may be better as there won't need to be any manual stripping of leading * in versions.

@jthegedus
Copy link
Contributor

jthegedus commented Jun 22, 2022

@jthegedus why did you select this PR over #1192? They are almost identical PRs. Unless there is something we want from #1192 it can be closed.

This for the asdf list command. #1192 was pitched as for the asdf list all <name> command. I didn't look closely at #1192 and didn't realise it could be leveraged for asdf list as well. I was just trying to close a PR, and this was an easy and old one to tackle.

As for *[:space:]<version>, that sounds logical to me. Currently asdf plugin list all uses <name>[:space:]*<url> with the * hugging the <url>. We might want to be consistent here and say that all flags/indicators of this type should be *[:space:]<val>? Updating this the code for asdf list is simple. Not sure how this intersects with #1192 now that this has been merged, sorry.

@Stratus3D
Copy link
Member

Currently asdf plugin list all uses [:space:]* with the * hugging the . We might want to be consistent here and say that all flags/indicators of this type should be *[:space:]?

I had forgotten about that. I guess [:space:]*<version> is fine for now.

Not sure how this intersects with #1192 now that this has been merged, sorry.

I'll take care of that one. Thanks!

@jthegedus
Copy link
Contributor

I had forgotten about that. I guess [:space:]* is fine for now.

Changing this to *[:space:]<version> would be better sooner than later. And making this change across the board would be ideal in one sweep.

Additionally, *[:space:]<version> would make parsing harder in that some rows are X columns and the ones with the flag are now X+1. Perhaps we should consider moving the flag to the start of the row so it is at least consistent where the additional column appears?

Currently we have:

# shows selected version of installed versions of tools
➜ asdf list
bats
  *1.7.0

# shows installed plugins
➜ asdf plugin list all
updating plugin repository...HEAD is now at df7ef4a chore: format README.md
...
bats                         *https://github.com/timgluz/asdf-bats.git
...

# after #1192 - shows installed versions
➜ asdf list all bats
...
1.6.0
1.6.1
*1.7.0

But perhaps it should be:

# shows selected version of installed versions of tools
➜ asdf list
bats
    1.6.0
  * 1.7.0

# shows installed plugins
➜ asdf plugin list all
updating plugin repository...HEAD is now at df7ef4a chore: format README.md
...
* bats                         https://github.com/timgluz/asdf-bats.git
...

# after #1192 - shows installed versions
➜ asdf list all bats
...
* 1.6.0
  1.6.1
* 1.7.0

@Stratus3D
Copy link
Member

@jthegedus another option might be trailing stars? e.g.

➜ asdf list all bats
...
1.6.0 *
1.6.1
1.7.0 *

That way if you want the version you know it's always first column, and if you want installed status, it's maybe the second column.

All this reminds me of git --porcelain, which is something I'd like to have (see #646 (comment) and #866 (comment)). Maybe we either omit this info entirely or format it differently when the --porcelain flag is provided to a command?

@jthegedus
Copy link
Contributor

A suffix marker might be the go.

I would also like to see --porcelain or some machine-specific flag. Since we both want to introduce this type of flag it would follow that we should have default output optimised for humans, so while annoying for scripting now, we could possibly leave this? 🤔

@Stratus3D
Copy link
Member

@jthegedus I have created #1291 for this.

@jsejcksn
Copy link
Contributor

Just wanted to give feedback that this change broke my parser. (I just subscribed to #1291 to follow along.)

@jthegedus
Copy link
Contributor

Just wanted to give feedback that this change broke my parser. (I just subscribed to #1291 to follow along.)

Sorry about that, unfortunately some breakages are unavoidable as we move towards a better design. Thanks for your patience.

@tomocrafter
Copy link

tomocrafter commented Dec 16, 2022

This pr seems introduced stack overflow and stack when running some shim command in specific directory (for me, it was go).
I try to investigate it more but Im happy if someone can help it.

@Stratus3D
Copy link
Member

@tomocrafter I just commented on #1372 this morning. That sounds like the same issue you are experiencing. This is due to an issue in the go plugin specifically.

For anyone who may be reading this: Plugins callbacks should never invoke asdf commands themselves. Doing so may result in infinitely loops of asdf -> plugin -> asdf -> plugin. Most plugins do not do this, but a few do. Features like this one may cause this to manifest as a stackoverflow or infinite loop.

@tomocrafter
Copy link

@Stratus3D Thank you for detailed explanation! and it really makes sense.

@sjml
Copy link
Contributor

sjml commented Dec 16, 2022

Just wanted to give feedback that this change broke my parser. (I just subscribed to #1291 to follow along.)

I don't know how helpful "me too" comments are, but just another voice that this broke an automated system I have for checking and updating my environment. Would be really nice to have old behavior available behind a flag for changes like this, but I know implementing that kind of thing is a pain. On the whole, thanks for the good work on this project.

@jsejcksn
Copy link
Contributor

Would be really nice to have old behavior available behind a flag for changes like this, but I know implementing that kind of thing is a pain.

@sjml If you want to follow the context/discussion of the separation of human- and machine-readable output formats, you can read and subscribe to #646 and #1291.

@log2
Copy link
Contributor

log2 commented Dec 27, 2022

Could asdf disable visual cues like this when it detects its output is piped in and not directed to user-facing terminal? Thank you

@botp
Copy link
Contributor Author

botp commented Dec 29, 2022

hi @log2, it is difficult and unreliable to detect pipes in shells, especially when passed thru ssh.

like @jthegedus, i too, do not like --porcelain behavior+name similar to git. i'd prefer --format=format. or maybe, as of now, we can try --delimiter

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.

Mark global versions in the output of list command
7 participants