Skip to content

Consult http_code and download_error_msg only for failed downloads. #137

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.

Sign up for GitHub

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 1 commit into from
Feb 5, 2021
Merged

Consult http_code and download_error_msg only for failed downloads. #137

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions src/dotnet-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ get_specific_version_from_version() {
local json_file="$5"

if [ -z "$json_file" ]; then
if [[ "$version" == "latest" ]]; then
if [[ "$version" == "latest" ]]; then
local version_info
version_info="$(get_latest_version_info "$azure_feed" "$channel" "$normalized_architecture" false)" || return 1
say_verbose "get_specific_version_from_version: version_info=$version_info"
Expand Down Expand Up @@ -522,7 +522,7 @@ construct_download_link() {
local specific_version="${4//[$'\t\r\n']}"
local specific_product_version="$(get_specific_product_version "$1" "$4")"
local osname="$5"

local download_link=null
if [[ "$runtime" == "dotnet" ]]; then
download_link="$azure_feed/Runtime/$specific_version/dotnet-runtime-$specific_product_version-$osname-$normalized_architecture.tar.gz"
Expand All @@ -542,7 +542,7 @@ construct_download_link() {
# azure_feed - $1
# specific_version - $2
get_specific_product_version() {
# If we find a 'productVersion.txt' at the root of any folder, we'll use its contents
# If we find a 'productVersion.txt' at the root of any folder, we'll use its contents
# to resolve the version of what's in the folder, superseding the specified version.
eval $invocation

Expand Down Expand Up @@ -753,9 +753,8 @@ download() {
elif machine_has "wget"; then
downloadwget "$remote_path" "$out_path" || failed=true
else
unset http_code
download_error_msg="Missing dependency: neither curl nor wget was found."
break
say_err "Missing dependency: neither curl nor wget was found."
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend that you return to the old logic here. There should be no http_code if there's no http request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that I should be unsetting it? We are exiting the script when we hit this condition though, so that doesn't matter unless I am missing something? http_code is only a global and not an env var.

FWIW it appears that the script was already exiting if neither curl or wget were present thanks to check_min_reqs().

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then maybe this is defensive. return 1 wouldn't exit here, but if check_min_reqs does it, then your original fix was better. Just means that the no-wget-or-curl branch of this was dead all along. Thanks for explaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I missed that about returning 1 from inside the function not actually exiting, thank you for catching that. I think it makes sense to be defensive also even if the branch is dead, and I think it's better to just exit with the message logged so I updated the PR to that fwiw.

exit 1
fi

if [ "$failed" = false ] || [ $attempts -ge 3 ] || { [ ! -z $http_code ] && [ $http_code = "404" ]; }; then
Expand Down Expand Up @@ -905,10 +904,10 @@ install_dotnet() {

# The download function will set variables $http_code and $download_error_msg in case of failure.
download "$download_link" "$zip_path" 2>&1 || download_failed=true
primary_path_http_code="$http_code"; primary_path_download_error_msg="$download_error_msg"

# if the download fails, download the legacy_download_link
if [ "$download_failed" = true ]; then
primary_path_http_code="$http_code"; primary_path_download_error_msg="$download_error_msg"
Copy link
Member

Choose a reason for hiding this comment

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

This will still be unbound if you don't have wget or curl - download will fail, but you won't have an error code. @bozturkMSFT does it make sense to say, just exit the script if neither was found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds reasonable enough to me at least so I updated the PR to do exactly that.

Copy link
Member

Choose a reason for hiding this comment

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

Returning 1 from download() will sadly preserve this error behavior. Exiting directly from that function wont .

There's two options:

  • Just log the message there and exit. There's little that can be done at any other point in the script that's useful to the users. This also avoids doing retries if there's no curl or wget.
  • Change this to something like:
Suggested change
primary_path_http_code="$http_code"; primary_path_download_error_msg="$download_error_msg"
primary_path_http_code="${http_code:-toolError}"; primary_path_download_error_msg="$download_error_msg"

The messages will repeat and do retries, but it's pretty small of a fix.

Even with this contribution I'd be happy, as most error cases in the script will be "couldn't download", not "couldn't find something to download with", and this would fix issues that are pretty disruptive right now.

case $primary_path_http_code in
404)
say "The resource at $download_link is not available."
Expand All @@ -928,9 +927,9 @@ install_dotnet() {

# The download function will set variables $http_code and $download_error_msg in case of failure.
download "$download_link" "$zip_path" 2>&1 || download_failed=true
legacy_path_http_code="$http_code"; legacy_path_download_error_msg="$download_error_msg"

if [ "$download_failed" = true ]; then
legacy_path_http_code="$http_code"; legacy_path_download_error_msg="$download_error_msg"
case $legacy_path_http_code in
404)
say "The resource at $download_link is not available."
Expand Down Expand Up @@ -1132,10 +1131,10 @@ do
echo " --arch,-Architecture,-Arch"
echo " Possible values: x64, arm, and arm64"
echo " --os <system> Specifies operating system to be used when selecting the installer."
echo " Overrides the OS determination approach used by the script. Supported values: osx, linux, linux-musl, freebsd, rhel.6."
echo " In case any other value is provided, the platform will be determined by the script based on machine configuration."
echo " Overrides the OS determination approach used by the script. Supported values: osx, linux, linux-musl, freebsd, rhel.6."
echo " In case any other value is provided, the platform will be determined by the script based on machine configuration."
echo " Not supported for legacy links. Use --runtime-id to specify platform for legacy links."
echo " Refer to: https://aka.ms/dotnet-os-lifecycle for more information."
echo " Refer to: https://aka.ms/dotnet-os-lifecycle for more information."
echo " --runtime <RUNTIME> Installs a shared runtime only, without the SDK."
echo " -Runtime"
echo " Possible values:"
Expand All @@ -1160,7 +1159,7 @@ do
echo " Installs just the shared runtime bits, not the entire SDK."
echo " --runtime-id Installs the .NET Tools for the given platform (use linux-x64 for portable linux)."
echo " -RuntimeId" The parameter is obsolete and may be removed in a future version of this script. Should be used only for versions below 2.1.
echo " For primary links to override OS or/and architecture, use --os and --architecture option instead."
echo " For primary links to override OS or/and architecture, use --os and --architecture option instead."
echo ""
echo "Install Location:"
echo " Location is chosen in following order:"
Expand Down Expand Up @@ -1197,7 +1196,7 @@ if [ "$dry_run" = true ]; then
if [ "$valid_legacy_download_link" = true ]; then
say "Legacy named payload URL: $legacy_download_link"
fi
repeatable_command="./$script_name --version "\""$specific_version"\"" --install-dir "\""$install_root"\"" --architecture "\""$normalized_architecture"\"" --os "\""$normalized_os"\"""
repeatable_command="./$script_name --version "\""$specific_version"\"" --install-dir "\""$install_root"\"" --architecture "\""$normalized_architecture"\"" --os "\""$normalized_os"\"""
if [[ "$runtime" == "dotnet" ]]; then
repeatable_command+=" --runtime "\""dotnet"\"""
elif [[ "$runtime" == "aspnetcore" ]]; then
Expand Down