Skip to content

[skip changelog] Improve install script's check for conflicting installation in $PATH #822

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

Merged
merged 2 commits into from
Jul 20, 2020
Merged

[skip changelog] Improve install script's check for conflicting installation in $PATH #822

merged 2 commits into from
Jul 20, 2020

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Jul 12, 2020

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • The PR follows our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

  • What kind of change does this PR introduce?

Bug fix


  • What is the current behavior?

The installation script's check for a conflicting prior installation in $PATH was prone to false positives. For example, any of these examples result in a spurious "An existing arduino-cli was found at..." error:

  • Paths differ by a redundant slash in $PATH:
    $ export PATH="$PATH:$PWD/bin/"
    $ curl -fsSL https://github.com/raw/arduino/arduino-cli/master/install.sh | sh
    ..
    An existing arduino-cli was found at /home/foo/bin//arduino-cli. Please prepend /home/foo/bin to your $PATH or remove the existing one.
    Failed to install arduino-cli
  • Paths differ by a redundant slash on custom installation path:
    $ export PATH="$PATH:$PWD/custom-path"
    $ curl -fsSL https://github.com/raw/arduino/arduino-cli/master/install.sh | BINDIR="$PWD/custom-path/" sh
    ...
    An existing arduino-cli was found at /home/foo/custom-path/arduino-cli. Please prepend /home/foo/custom-path/ to your $PATH or remove the existing one.
    Failed to install arduino-cli
  • Use of relative paths:
    $ export PATH="$PATH:$PWD/relative-path"
    $ curl -fsSL https://github.com/raw/arduino/arduino-cli/master/install.sh | BINDIR="./relative-path" sh
    ...
    An existing arduino-cli was found at /home/foo/relative-path/arduino-cli. Please prepend ./relative-path to your $PATH or remove the existing one.
    Failed to install arduino-cli
  • Symlink:
    $ export PATH="$PATH:/home/foo/symlink-path"
    $ ln -s "$PWD/bin" "/home/foo/symlink-path"
    $ curl -fsSL https://github.com/raw/arduino/arduino-cli/master/install.sh | sh
    ...
    An existing arduino-cli was found at /home/foo/symlink-path/arduino-cli. Please prepend /home/foo/bin to your $PATH or remove the existing one.
    Failed to install arduino-cli

When the check for conflicting installation in $PATH has a positive, a "Failed to install arduino-cli" error message is displayed, even though the installation was successful.

When the check for conflicting installation in $PATH has a positive, the script has exit status 1, even though this positive does not really represent an error.


  • What is the new behavior?

The check for conflicting installation in $PATH is made more robust by comparing the resolved, absolute paths.

When the check for conflicting installation in $PATH has a positive, no confusing "Failed to install arduino-cli" error message is displayed.

When the check for conflicting installation in $PATH has a positive, it prints a helpful warning, but the script's exit status is 0.


  • Does this PR introduce a breaking change?

The script no longer returns exit status 1 when a conflicting installation in $PATH is detected. This is a breaking change for anyone relying on the previous behavior. However, I think this is unlikely.


  • Other information:

If you want to test the modified script using the recommended installation method, the command is:

curl -fsSL https://github.com/raw/per1234/arduino-cli/fix-install-script/install.sh | sh

My initial instinct for improving the comparison was to use the -ef file test operator, but that is bash-specific while the install script specifies sh.


It didn't end up being relevant to this work, but I'll add the information here for future reference since I had trouble finding it: The original base for the install script is here: https://github.com/Masterminds/glide.sh/blob/master/get, not in the repository mentioned by the comment in the script.

per1234 added 2 commits July 11, 2020 22:07
…eliable

Previously, the comparison between the install script's target path and the path of an existing installation in $PATH could easily give a false positive, producing a spurious "An existing arduino-cli was found at..." error. The solution is to fully resolve the paths before comparing them.
…tected

Previously, when a different installation of Arduino CLI was detected in $PATH by the installation script, the error message "Failed to install arduino-cli" would be printed and the script would have exit status 1.

It is wise that the script warns of this situation ("An existing arduino-cli was found at..."), but the "Failed to install arduino-cli" error message was misleading, since Arduino CLI was indeed installed. It is also incorrect to have an error exit status in this situation, since the user may intend to have multiple installations and provide the full path when using the new installation.
@per1234 per1234 changed the title [skip changelog] Fix install script [skip changelog] Improve install script's check for conflicting installation in $PATH Jul 12, 2020
@per1234 per1234 merged commit e41e22d into arduino:master Jul 20, 2020
@per1234 per1234 deleted the fix-install-script branch July 20, 2020 21:40
@per1234 per1234 self-assigned this Nov 23, 2021
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.

2 participants