Skip to content

added --os option to bash script #118

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
Dec 10, 2020

Conversation

vlada-shubina
Copy link
Member

@vlada-shubina vlada-shubina commented Dec 4, 2020

Implements suggestion in: #13 (comment)
--os option allows user to specify the OS to be used when selecting the installation package.

Important notes to review:

  • the implementation is done only for primary links. For legacy links, there is a wider range of supported OS, and I don't want to mix both sets together. I suggest those who want to override OS for legacy links may still use --runtime-id.
  • the allowed values for --os are: linux, linux-musl, osx, freebsd, rhel.6 - aligned with possible choices for OS auto-detection. I'm not sure about including freebsd and rhel.6 - I haven't seen and freebsd specific packages, and packages for rhel.6 are rare. According to https://github.com/dotnet/installer rhel.6 packages are only available for 3.1. As alternative, we can include them but add warning that some versions might not support them.

Powershell script is hardcoded to use "win" as OS, I see no point in making --os version for powershell.

@@ -40,7 +40,7 @@ if [ -t 1 ] && command -v tput > /dev/null; then
fi

say_warning() {
printf "%b\n" "${yellow:-}dotnet_install: Warning: $1${normal:-}"
printf "%b\n" "${yellow:-}dotnet_install: Warning: $1${normal:-}" >&3
Copy link
Member Author

Choose a reason for hiding this comment

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

this fix is needed so warnings are done to standard output (see line 20)

@@ -1074,6 +1109,11 @@ do
echo " --architecture <ARCHITECTURE> Architecture of dotnet binaries to be installed, Defaults to \`$architecture\`."
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."
Copy link
Member Author

Choose a reason for hiding this comment

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

This list doesn't include supported OS for legacy links. As mentioned in 1st comment I don't want to mix them, as it increases chance of error.
My suggestion for those who still on legacy versions to continue to use --runtime-id to override OS if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable to me.

@vlada-shubina vlada-shubina requested a review from a team December 7, 2020 14:30
…ecture is given

replaced legacy links reference in help with versions below 2.1
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.

3 participants