Fix CLI upgrade bug

[DavidObando]

Before this change, KoreBuild would not pick the correct version when it had to upgrade. The variables used to point to the latest version (in case an upgrade is needed) hadn't been updated before download commences. This fixes that situation and allows the machine to have the latest CLI from the beta channel installed.

@sajayantony @muratg @CesarBS @pranavkm @victorhurdugaci

[dnfclas]

Hi @DavidObando, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[pranavkm]

Doesn't the url crafted on line 172 work? If you set it to Latest it should grab https://dotnetcli.blob.core.windows.net/dotnet/dev/Binaries/Latest/dotnet-osx-x64.latest.tar.gz which works?

[DavidObando]

In this case it's because KoreBuild.sh sends the -version argument to dotnet-install.sh with value 1.0.0.001244. That is, we're not moving to "Latest" every time you build.

The changes I'm proposing are simply doing what the script was supposed to do on a version change. Assume your computer has dotnet version 1.0.0.001244 but we change KoreBuild.sh to point to version 1.0.0.1333. The script in its current form will always download the wrong file and fail to upgrade you to the newly specified version.

[victorhurdugaci]

The change looks okay but what I don't like is making changes in this file since this it's a copy of https://github.com/dotnet/cli/tree/rel/1.0.0/scripts/obtain and they files will diverge (even more).

If the CLI decides to update the way they install, we're going to have to merge all our changes... We should fix this instead: https://github.com/dotnet/cli/issues/1091 @DavidObando, want to give it a try? :)

[DavidObando]

@victorhurdugaci I can backport these changes to dotnet/cli. The changes are just around consistency, and it helps us not have to re-download the cli every time we build in Linux (which is happening today).

This change helps ensure that #384 can be properly fixed.

[pranavkm]

Why does, dotnet-win-x64.latest.zip not work? We shouldn't need to specify the Latest version: #383 (comment)

[DavidObando]

The same bug was present in the powershell script, as in the bash script. This just brings both scripts to functional parity.

[pranavkm]

⌚

[victorhurdugaci]

@Sridhar-MS @piotrpMSFT how do you feel about these changes? Should be port them back to your install scripts and fix https://github.com/dotnet/cli/issues/1091

[TheRealPiotrP]

If the change is needed we should make it in the CLI repo as well.

I'm not sure I understand the need for this change, however. Why do we need explicit version numbers in filename/URL when we're asking for 'latest'? Is the intent to avoid issues if a newer build gets dropped between the time that version is retrieved and the time that the zip is retrieved?

[DavidObando]

@piotrpMSFT If simply asking for 'latest' is what's intended, then I can revert those changes and also remove the code that hits https://dotnetcli.blob.core.windows.net/dotnet/$CHANNEL/dnvm/latest.$os.version asking for the specific version to download. What's your take on this? That code that asks for the latest version of dnvm comes from scripts/obtain in dotnet/cli.

[TheRealPiotrP]

We shouldn't remove that code.

The purpose of the request for latest.$os.version is to compare to what is already available locally. We need to know if what was downloaded before as latest is still the latest. If those versions are the same then we do nothing. Otherwise, we download the new latest and update the local version number.

Makes sense?

[DavidObando]

It does make sense, except that's not what I'm observing in my Ubuntu VM. KoreBuild.sh is hardwired to send --version 1.0.0.001244, meaning, it's not looking for latest.

Before these changes I'm proposing, dotnet-install.sh will always say: hey, I see your local version is 1.0.0.001244 and latest is 1.0.0.1370, downloading version 1.0.0.001244. It then proceeds to download that version and install it again, regardless of what I have installed.

We're essentially re-installing the same version of dotnet every time we build.

[TheRealPiotrP]

Looking at the code:

 if (Test-Path $LocalFile)
 {
    ...
     if ($LocalVersion -and $LocalHash)
     {
         if ($Version -eq "Latest")
         {
             ...
             download the latest version file
             ...
         }
          elseif ($LocalVersion -eq $Version)
          {
             say "$Version is already installed."
             exit 0
         }
     }
     ....
     download the version requested
 }

So it looks like the script will only download the version file IF Latest is specified as the version. Otherwise, it will get the specific version you're asking for...

[DavidObando]

That's true for PowerShell but not for Bash.

[TheRealPiotrP]

ahh, ok, I get it. Can this PR be moved over to the CLI repo and simply update .sh to match .ps1?

[DavidObando]

You mean without the override to dotnet_url and dotnet_filename? I can do that. I still think the override is necessary, but I'll do as you say.

[TheRealPiotrP]

well, what's the intent there? why do we want to calculate these?

[DavidObando]

Currently we have this:

local remoteData="$(curl -s https://dotnetcli.blob.core.windows.net/dotnet/$CHANNEL/dnvm/latest.$os.version)"  
[ $? != 0 ] && say_err "Unable to determine latest version." && return 1  

local remoteVersion=$(IFS="\n" && echo $remoteData | tail -n 1)  
local remoteHash=$(IFS="\n" && echo $remoteData | head -n 1) 

The code is checking what the latest version of dnvm is, and checks against its checksum to see if we have the latest. Then we proceed to download from:

local dotnet_url="https://dotnetcli.blob.core.windows.net/dotnet/$CHANNEL/Binaries/$VERSION"

I'd argue that these could go out of sync and cause the same behavior of re-installing on every build even if you already have the latest. If you think this is okay, I'll go for it. Otherwise we have two alternatives.

1. Either we remove the check against dnvm and somehow check against the .version file of the latest dotnet, or
2. We override dotnet_url to point to the version indicated by dnvm's version file.

By the way, thanks for giving this some of your time.

[TheRealPiotrP]

That dnvm string is a relic we should have cleaned up... the source for these scripts came out of dnvm and so the server-side paths kept the old dnvm string. The version number in these files is the version of the dotnet cli and not of dnvm. This confusion aside, it seems like we're actually doing (1) above?

[DavidObando]

Yeah, the current code will work as long as $CHANNEL/dnvm/latest.$os.version is up-to-date with $CHANNEL/Binaries/$VERSION/dotnet-$os-x64.latest.tar.gz. That's the assumption I didn't want to make. But from what I get from you this is probably not a concern.

[DavidObando]

Hey @sajayantony, this fix also improves build time by a few seconds in Linux and OSX as it avoids re-downloading and installing the CLI if you already have it. Today the CLI gets downloaded again every time you build, even though the version of the CLI we use is fixed.

@piotrpMSFT @victorhurdugaci how do you want to proceed with this?

[TheRealPiotrP]

@krwq and @blackdwarf are in the process of productizing the script files. At this point, I think we should just wait.

[DavidObando]

@piotrpMSFT sounds good, thanks!

[victorhurdugaci]

If you get to merge this, please make the same change in github.com/aspnet/KoreBuild

[TheRealPiotrP]

FYI, please take a look at @blackdwarf and @krwq's PR dotnet/cli#1482. It details how the updated install scripts will work.

[muratg]

What's the latest on this PR? Needed still?

[DavidObando]

@muratg I don't think so. I was waiting for changes to come into the build and I saw them yesterday. I'm closing as this is not necessary anymore. Thanks!