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

asdf install <plugin> <version> should return a non-zero status code if already installed #747

Closed
jsejcksn opened this issue Jun 17, 2020 · 12 comments
Assignees

Comments

@jsejcksn
Copy link
Contributor

Similar to #322 — but for asdf install <plugin> <version>. It should exit with a non-zero status code if the version was already installed.

~ % asdf list deno
  1.1.0

~ % asdf install deno 1.1.0
deno 1.1.0 is already installed

~ % echo $?
0  # should be another value because the installation did not take place
@jthegedus
Copy link
Contributor

jthegedus commented Jun 17, 2020

asdf install uses the .tool-versions file to install each version in the config file with the same install_tool_version function. Should it return non-zero when any install fails? Should it be different when all installs fail?

As you linked, we had this request for asdf plugin add <plugin> in the past. What other commands do you feel we should support this behaviour with?

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Jun 18, 2020

What other commands do you feel we should support this behaviour with?

I would need to study the list of commands to give a more considered answer, but here's my precursory response:

asdf plugin add
asdf plugin remove
asdf plugin update
asdf install
asdf uninstall
asdf reshim
asdf update

I think the logic of calling commands should be

if:
the expected action doesn't occur

then:
a non-zero exit code should be returned, and preferably one that indicates the following statement:

In the example I posted, the resulting filesystem state is essentially identical to what it would be if the action had succeeded, but the actual action did not take place because it didn't need to.

Perhaps you might conclude that those two things are not different enough to warrant a different code, and that returning a 0 on install is appropriate if the version is already installed. But that should also be the case for plugins, etc. as well.

Here's an analogous example from a different domain—http status codes:

A 2xx series code means success. (equivalent to 0 in the shell)

A 3xx series code means client redirection. 304 is not a success code, but it means that the request was extraneous and that the client should use the cached copy of the requested data.

I think that there are going to be different opinions on this, and that's ok, but I argue that consistency is a global expectation.

@jthegedus
Copy link
Contributor

jthegedus commented Jun 18, 2020

I think that there are going to be different opinions on this, and that's ok, but I argue that consistency is a global expectation.

I agree we should be consistent. Do you know of any other Shell projects that have a specific exit code table?

asdf current <plugin> is another command that can currently return a non-zero exit code.

@jsejcksn
Copy link
Contributor Author

Do you know of any other Shell projects that have a specific exit code table?

@jthegedus Here's a very similar question on UNIX SE: https://unix.stackexchange.com/questions/110348/how-do-i-get-the-list-of-exit-codes-and-or-return-codes-and-meaning-for-a-comm

One answer includes a link to info about wget.

@jthegedus
Copy link
Contributor

jthegedus commented Jun 18, 2020

#310 had a link to the BASH Advanced Scripting Guide which lists the following codes:

Exit Code Number Meaning Example Comments
1 Catchall for general errors let "var1 = 1/0" Miscellaneous errors, such as "divide by zero" and other impermissible operations
2 Misuse of shell builtins (according to Bash documentation) empty_function() {} Missing keyword or command, or permission problem (and diff return code on a failed binary file comparison).
126 Command invoked cannot execute /dev/null Permission problem or command is not an executable
127 "command not found" illegal_command Possible problem with $PATH or a typo
128 Invalid argument to exit exit 3.14159 exit takes only integer args in the range 0 - 255 (see first footnote)
128+n Fatal error signal "n" kill -9 $PPID of script $? returns 137 (128 + 9)
130 Script terminated by Control-C Ctl-C Control-C is fatal error signal 2, (130 = 128 + 2, see above)
255* Exit status out of range exit -1 exit takes only integer args in the range 0 - 255

So whatever we come up with should use these first and avoid clashes for any custom codes we define.

@Stratus3D
Copy link
Member

@jthegedus jthegedus self-assigned this Jun 27, 2020
@dideler
Copy link

dideler commented Sep 13, 2020

I'm -1 on this proposal and would rather keep non-zero exit codes for actual errors.

I find it surprising behaviour when an idempotent command has a different exit code on subsequent runs.
If it's a no-op and it doesn't introduce problematic state for the user, why treat it as an error?

If the user wanted to avoid running a no-op command, they could instead check if the condition is already satisfied. E.g.

if asdf plugin list | grep -q nodejs; then
  echo "nodejs plugin already installed"
else
  asdf plugin-add nodejs
fi

But I find it hard to imagine a scenario where they'd actually want to do that. In most cases this simple code is sufficient

# Install plugin or do nothing if already installed, and install latest nodejs version
asdf plugin-add nodejs && asdf install nodejs latest

Because the end state matters, having the plugin be present. Doesn't matter if it was installed or not beforehand. By treating the no-op scenario as a failure, it forces users to add defensive code to their scripts even though the outcome of running the command is the same.

With the non-zero exit code, the quickest workaround likely to be used is || true which is not safe as it'll swallow real errors.

Unfortunately I don't think there is a standard to reference, you'll find common tools with opposite behaviour.

E.g. BSD's chsh returns 0 when changing the shell has no effect

$ echo $SHELL
/usr/local/bin/fish
$ chsh -s /usr/local/bin/fish
Changing shell for dideler.
Password for dideler:
chsh: no changes made
$ echo $?
0

And others, like Linux's useradd return a non-zero exit code.

$ useradd foo
$ useradd foo
useradd: user 'foo' already exists
$ echo $?
9

Ultimately I think it's more important that asdf be consistent with exit codes across its subcommands, rather than what the value of the exit code is. But wanted to chime in with a different perspective before this issue is addressed.

dideler added a commit to dideler/setup-macos that referenced this issue Sep 13, 2020
Related to
- asdf-vm/asdf#659
- asdf-vm/asdf#688

Note that `asdf install` may soon also return a non-zero code [1] which
would require a similar patch in these language scripts.

[1]: asdf-vm/asdf#747
@jthegedus
Copy link
Contributor

jthegedus commented Sep 14, 2020

@dideler Thanks for your input, feature requests too often only reflect the supporters of the changes.

History of these ideas in this project:

Looking over the previous requests for this "different exit code" feature(s) we haven't had many people contribute support or otherwise, only the requester has been involved in the discussions. Perhaps this is indicative of it not being particularly important or us implementing changes too soon?

@Stratus3D what are your thoughts? Given we reverted the last request of this type?

@jsejcksn happy to hear further thoughts of yours since this initial request.

I think we should be consistent. Given we have a different set of return codes for asdf current <plugin> and (potentially?) asdf plugin add then we should either:

  • remove the existing two custom exit codes
  • determine a rule-set for all commands with codes as in the wget example above

Reverting the two existing seems easiest, but obviously breaking.

This is definitely a problem that needs a resolution before we even think of a 1.0.0 release.

@jsejcksn
Copy link
Contributor Author

It seems that everyone agrees about consistency 🤝

Defining expectations is central to the decision-making process, because I think we all occasionally assume that everyone is sharing a perspective.

@dideler brings up some interesting points:

I find it surprising behaviour when an idempotent command has a different exit code on subsequent runs.
If it's a no-op and it doesn't introduce problematic state for the user, why treat it as an error?

I think the definitions of idempotence and no-op really depend on your expectations of what the command is, but I'm not sure everyone agrees about that.

Here's a quote from issue #322:

Expected behavior

Trying to install a plugin that's already added should return a different code than trying to install a plugin that doesn't exist. This will make it easier to script asdf installations in prod environments. It doesn't really matter what the exit code is; just that it's different from a non-existent plugin.

IMHO, if the plugin's already there, asdf should return 0. The user assumes calling asdf plugin-add <plugin> will result in asdf having that plugin if the call was successful. If the plugin's already there, that assumption is not violated. Therefore the return code should be successful (0).

Actual behavior

Attempting to install plugins that are already added and installing plugins that don't exist both have the same return code of 1.

In the description he states his assumption that what the command does "will result in asdf having that plugin". Let's apply that to the install command, and look at some different ways that idempotent and no-op can be viewed:

But first, I want to mention that I agree that the exit codes for a failure and a no-op (however that's defined) should not be the same. I think everyone agrees on this, but if not, please share your perspective!

According to the quoted description above, in the author's perspective, what the install command does is unimportant, but the resulting availability state of the plugin after running the command is how he understands it. In that view, the availability state of the plugin before running the command is also irrelevant. From that point of view, the command is idempotent and running it when the plugin is already installed would result in a no-op.

Another point of view is making a distinction about what the command does—which is "actually install the plugin". In that view, if the plugin is already installed, the result of the command would be a no-op, but the command should not be considered to have succeeded because an installation didn't take place. This also yields information about the installation state of the plugin before running the command.

In regard to consistency, I'd like to reference this SE question: Non-zero exit status for clean exit.

Ultimately, I think that—in effort of simplicity and high compatibility with the rest of the UNIX ecosystem—it makes sense for asdf to return 0 as the exit status code if the plugin is installed after running the command (whether or not it was installed before).

@HashNuke
Copy link
Member

HashNuke commented Oct 7, 2020

Quoting @dideler

I find it surprising behaviour when an idempotent command has a different exit code on subsequent runs.
If it's a no-op and it doesn't introduce problematic state for the user, why treat it as an error?

I'm building a deployment tool in my spare time and I can vote for that

  • Both scenarios would be expected to return a 0 exit code: For first run that installs the package and subsequent runs that don't really do much if the package is installed.
  • Most exit status checks are simple to point where non-zero are considered as not success, where in here the command was a success as far the existence of the package is concerned.

For the sake of simplicity, we can stick with the existing behaviour. Closing this issue. Please feel free to continue discussion and reopen if appropriate.

@HashNuke HashNuke closed this as completed Oct 7, 2020
@jthegedus
Copy link
Contributor

My only further input would be to consider reverting the other custom exit codes, otherwise we will receive this request again, and it's a reasonable request with the precedent set

@ghostsquad
Copy link

Another point of view is making a distinction about what the command does—which is "actually install the plugin". In that view, if the plugin is already installed, the result of the command would be a no-op, but the command should not be considered to have succeeded because an installation didn't take place. This also yields information about the installation state of the plugin before running the command.

I believe that this perspective is a LOT less useful in real world situations. From a theoretical standpoint, it does seem useful to know what's going on behind the scenes. But when you actually get down to writing automation, the idea of "reconciliation" ends up being far superior to anything else. Another way to state this, is that as a user, I don't care about the how, I care about the what.

In the case of how vs what as it pertains to plugin add

what plugin is installed
how <insert code, if statements, etc, nobody really cares>

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

No branches or pull requests

6 participants