dotnet / install-scripts

MIT License
144 stars 76 forks source link

Fix net10.0 detection for osx-arm64 #526

Closed am11 closed 1 month ago

am11 commented 1 month ago

It was downloading osx-x64 on osx-arm64 machine due to relaxed pattern matching:

$ ./dotnet-install.sh --quality daily --channel "10.0" --install-dir ~/.dotnet10 -verbose
dotnet-install: Note that the intended use of this script is for Continuous Integration (CI) scenarios, where:
dotnet-install: - The SDK needs to be installed without user interaction and without admin rights.
dotnet-install: - The SDK installation doesn't need to persist across multiple CI runs.
dotnet-install: To set up a development environment or to run apps, use installers rather than this script. Visit https://dotnet.microsoft.com/download to get the installer.

dotnet-install: Calling: machine_has curl
dotnet-install: Calling: calculate_vars 
dotnet-install: Calling: get_normalized_architecture_from_architecture <auto>
dotnet-install: Calling: get_machine_architecture 
dotnet-install: Normalized architecture: 'arm64'.
dotnet-install: Calling: get_normalized_os 
dotnet-install: Calling: get_current_os_name 
dotnet-install: Normalized OS: 'osx'.
dotnet-install: Calling: get_normalized_quality daily
dotnet-install: Normalized quality: 'daily'.
dotnet-install: Calling: get_normalized_channel 10.0
dotnet-install: Normalized channel: '10.0'.
dotnet-install: Calling: get_normalized_product 
dotnet-install: Normalized product: 'dotnet-sdk'.
dotnet-install: Calling: resolve_installation_path /Users/adeel/.dotnet10
dotnet-install: InstallRoot: '/Users/adeel/.dotnet10'.
dotnet-install: Calling: get_normalized_architecture_for_specific_sdk_version Latest 10.0 arm64
dotnet-install: Calling: get_current_os_name 
dotnet-install: Changing user architecture from 'arm64' to 'x64' because .NET SDKs prior to version 6.0 do not support arm64.
...

This delta fixes the issue by parsing major version from the input and comparing if it is less than 6.

am11 commented 1 month ago

@baronfel, could we please deploy this to dot.net after the merge? I’m concerned that as people start updating to the 10.0 previews, they may run into some unexpected issues on osx-arm64. I discovered this myself (after a cup of β˜•) because the use of Rosetta emulation isn't clearly highlighted, which might lead to confusion about the fact that .NET is actually emulating. πŸ˜… Thanks!

baronfel commented 1 month ago

I've asked the team to review, and once we merge we'll definitely work with @mairaw's team to get it updated πŸ‘

MichalPavlik commented 1 month ago

I'm not a Bash person, but it looks good. I would like to add one small change to cover another issue and then I can start the deployment process. It takes some time as we need to test the new version in 4 major .NET repos before publishing to dot.net. But I think it could be done this week.

am11 commented 1 month ago

Thank you! πŸŽ‰

MichalPavlik commented 1 month ago

Thanks for the PR @am11 :)