Fix installation script bugs affecting Apple M1 users #376
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The installation script contained a `get()` function used to make an HTTP request. The response body is required by the caller, but shell functions do not support returning content as you might do when using a more capable programming language. As a workaround, the script author set up a system where the caller passed an arbitrary variable name to the function, which the function then assigns with the response body string. This was done using the `eval` builtin. The `eval` command was quoted in a naive manner, not considering that the body content could contain any arbitrary characters. This made it possible that contents of the response body could be unintentionally executed on the user's machine. In the context of the installation script, this happened under the following conditions: - The release download was not available from Arduino's downloads server - The response body from the Releases GitHub API request contained single quotes The most minimal fix would likely have been to change the `eval` command so that the variable containing the response body text was not expanded in the command: eval "$1=\$GET_BODY" However, this problem has provided clear evidence that best practices are to avoid `eval` altogether unless absolutely necessary. Since it is only called once, the entire function is not necessary (and in fact it is questionable whether there is any real value in the entire GitHub releases fallback system). So I decided the best fix was to do away with the function altogether, replacing its single call with the contents of the former function. This removed unnecessary complexity from the script without any decrease in efficiency or increase in maintenance burden.
Apple's Rosetta 2 software allows applications built for x86-64 hosts to run on machines using their ARM 64-bit M1 processor. Due to current infrastructure challenges (e.g., lack of GitHub-hosted GitHub Actions runners), Arduino Lint is not yet available as a native M1 build, yet the existing macOS x86-64 builds work perfectly well on these machines thanks to Rosetta 2. The installation script previously failed to install Arduino Lint when ran on the M1 host: Did not find a release for your system: macOS arm64
The installation script contains a `checkLatestVersion()` function which is used to determine the latest version of the project when the user does not specify a version to install. The version number is required by the caller, but shell functions do not support returning such content. As a workaround, the script author set up a system where the caller passed an arbitrary variable name to the function. The function then sets a global variable of that name with the release name. So it resembles a "pass by reference" approach, but isn't. This was done using the `eval` builtin. This tool must be used with caution and best practices is to avoid it unless absolutely necessary. The system used by the script has some value for a reusable function. However, in this case the function is only intended for internal use by the script, and is called only once. So the unintuitive nature of this system and potential for bugs don't bring any benefits when compared with the much more straightforward approach of simply using a fixed name for the global variable used to store the release name.
per1234
added
topic: infrastructure
umbynos
approved these changes
May 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An installation script contained in this repository provides convenient installation of Arduino CLI:
https://arduino.github.io/arduino-lint/dev/installation/#use-the-install-script
Some defects are present in the script's handling of lack of a build for a specific host architecture, which manifested when users with Apple M1 systems attempted to use it:
The script is hosted and maintained in a separate repository as part of a collection of reusable tooling project assets.
The defects in the script were fixed, and some general improvements made to the related code, in that repository:
arduino/tooling-project-assets@127116b#gitext://gotocommit/127116b30163e5c63384d6a46c57f9f5d093d61a
arduino/tooling-project-assets@e2b5740#gitext://gotocommit/e2b5740daf890aa20d27d901ee8fc11743b38036
arduino/tooling-project-assets@d8c59e3#gitext://gotocommit/d8c59e3caac6182077e9bbefe3a5209c3ec96782
Those are pulled into the Arduino Lint project here.