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

Flexibilize release asset selection #2

Closed
wants to merge 38 commits into from

Conversation

AZMCode
Copy link

@AZMCode AZMCode commented May 25, 2020

The plugin can now adjust to naming convention flexibility in stedolan/jq's release names.
Fixes #1

The plugin can now select between various files available in a release in a smarter way.
Fixes #1
@AZMCode
Copy link
Author

AZMCode commented May 25, 2020

Tested that it worked in Ubuntu 64-bits.
The amount of errors printed when you try to download a nonexistent release might be a bit too much, and so might need a bit of work.

Edit: This happens because each function used in the process logs its own error. There might be better ways to handle this, but no obvious ones I can see without sacrificing useful error output, given bash's lack of error handling capability.

Copy link
Author

@AZMCode AZMCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented and explained the changes done in the commit.

@@ -8,6 +8,7 @@ set -o nounset
set -o pipefail

readonly BINARY_NAME="jq"
readonly RELEASES_URL="https://api.github.com/repos/stedolan/jq/releases"
Copy link
Author

@AZMCode AZMCode Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added constant for RELEASES_URL in the same way the list-all script does

@@ -18,23 +19,109 @@ error_exit() {
exit "${2:-1}"
}

platform() {
get_platform() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the platform() function to get_platform(), in line with other functions added in the PR

bin/install Outdated
local -r os=$(
case "$(uname | tr '[:upper:]' '[:lower:]')" in
darwin) echo "osx" ;;
*) echo "linux" ;;
esac
)

echo "${os}-amd64"
echo "${os}"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this function is no longer responsible of directly choosing the release asset to be installed, it now only returns the platform name, either osx or linux

}
get_arch(){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_arch() function returns the architecture of the system, returning '32' for x86 and compatibles, as well as '64' for x86_64

error_exits if the release if uname -m does not return one of x86_64, i386, or i686

return
}

get_assets_url() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function get_assets_url(install_version) returns the Github API endpoint that itself returns all available assets for a given release. This is done by querying the releases URL, extracting an array of all 'assets_url' and 'tag_name' properties within the json returned, putting them in two arrays, and correlating the two arrays.

Hacky solution, but it seems to work, given the absence of jq in the runtime system at the time of execution.

error_exits on a release not found.

Have not tested what happens when the input is an empty string.

done
error_exit 'Given version did not match any releases. Try list-all to see available options'
}
find_all_asset_names() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find_all_asset_names(install_version) calls get_assets_url($install_version) to get the endpoint for the release of the given version, then extracts the values of all 'name' properties within the JSON, and returns them in an array.

Due to the way arrays work in bash scripting, unexpected behaviors might happen is a release asset contains a space in its name.

echo ${asset_names[@]}
return
}
filter_assets() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter_assets(inArr,platform,arch) takes an array of release asset names, and returns all that match the given platform and architecture as an array.

The platform argument takes any value returned by get_platform(), and the arch argument takes any value returned by get_arch(). Other values have not been tested.

Values which do not match the following perl-style regexes are excluded:

  • $platform.*((86(?!_64))|32) for an arch value of 32
  • $platform.*(64) for an arch value of 64

bin/install Outdated
done
echo "${outArr[@]}"
}
find_file_name() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function find_file_name(install_version) returns a fitting release asset for the given install_version, calling find_all_asset_names($install_version) to get all available assets in the given version, then filtering them with filter_assets.

  • If no fitting assets are returned from the previouslt called functions, it will error_exit, listing the platform and architecture.
  • If a single fitting asset is encountered, it returns the asset name silently.
  • If more than one fitting asset is encountered, it logs to stderr to let the user know that multiple useable assets were found, and returns the first one found.

bin/install Outdated
install() {
local -r install_type="$1"
local -r install_version="$2"
local -r install_path="$3"
local -r install_path_bin="${install_path}/bin"
local -r download_url="${DOWNLOAD_BASE_URL}/${install_version}/${BINARY_NAME}-$(platform)"
declare file_name="$(find_file_name "$install_version")"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the install(install_type,install_version,install_path) function is modified to, before defining the local download_url variable, call find_file_name($install_version) to find out if there exists a fitting asset.

  • If no asset is found, install now error_exits.
  • If an asset is found, the file_name variable now contains the needed string, which is then used to complete download_url, and the function proceeds as normal.

install "$ASDF_INSTALL_TYPE" "$ASDF_INSTALL_VERSION" "$ASDF_INSTALL_PATH"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental newline. What is the standard in this project on newlines at the end of files?

@AZMCode AZMCode changed the title Smarter release asset selection Flexibilize release asset selection Jun 2, 2020
@AZMCode
Copy link
Author

AZMCode commented Aug 10, 2020

Why would it fail on macOS?
Can someone with a mac help me debug?

@AZMCode AZMCode marked this pull request as draft August 10, 2020 12:06
Program now takes version input and output without jq prepended.
In progress of achieving OSX compatibility
Signed-off-by: AZMCode <adrianozambrana@protonmail.com>
Signed-off-by: AZMCode <adrianozambrana@protonmail.com>
…KEN.

Don't get any ideas, the pass is already revoked XD

Signed-off-by: AZMCode <adrianozambrana@protonmail.com>
@AZMCode AZMCode marked this pull request as ready for review October 5, 2020 18:38
@AZMCode AZMCode mentioned this pull request Dec 4, 2020
@AZMCode AZMCode closed this Dec 5, 2020
@AZMCode AZMCode deleted the patch-1 branch December 5, 2020 14:49
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.

Linux install fails
1 participant