Skip to content

Use "-Channel x.0 -Quality Daily" option with dotnet-install for configure.ps1#4289

Merged
erdembayar merged 5 commits intodevfrom
dev-eryondon-971
Oct 1, 2021
Merged

Use "-Channel x.0 -Quality Daily" option with dotnet-install for configure.ps1#4289
erdembayar merged 5 commits intodevfrom
dev-eryondon-971

Conversation

@erdembayar
Copy link
Contributor

@erdembayar erdembayar commented Sep 29, 2021

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/971

Regression? Last working version:

Description

Currently when we run configure.ps1 with dotnet-install.ps1 -Channel release/5.0.2xx then getting warning. Instead we run with dotnet-install.ps1 -Channel 5 -Quality Daily as warning is suggesting.
Before: https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=5249917&view=logs&j=d80b2849-30d4-52dd-74cd-d6536ba98fe0&t=b4c8a850-ec2f-53a4-6259-6271fa5e03fb&l=25
After: Now I don't see that warning anymore.
https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5269260&view=logs&j=d80b2849-30d4-52dd-74cd-d6536ba98fe0&t=b4c8a850-ec2f-53a4-6259-6271fa5e03fb&l=20

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@erdembayar erdembayar marked this pull request as ready for review September 29, 2021 20:28
@erdembayar erdembayar requested a review from a team as a code owner September 29, 2021 20:28
build/common.ps1 Outdated
if ($Force -or -not (Test-Path $probeDotnetPath)) {
Trace-Log "$DotNetInstall -Channel $($cli.Channel) -InstallDir $($cli.Root) -Version $($cli.Version) -Architecture $arch -NoPath"
& $DotNetInstall -Channel $cli.Channel -InstallDir $cli.Root -Version $cli.Version -Architecture $arch -NoPath
$channelMainVersion = "5"
Copy link
Member

Choose a reason for hiding this comment

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

We should be depending on 6.0 right now, so the channel probably needs to be interpreted different.

Copy link
Member

Choose a reason for hiding this comment

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

We specify 5.0.2xx right now, but we should be using 6.0.1xx. So we can'treally hard code 5 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it need to be future proof. Here 5 is just default placeholder value, but actual value is calculated from $cli.Channel variable and override it inside foreach loop (Line 244-251). Also $cli.Channel itself is coming from build\config.props.
msbuild build\config.props /v:m /nologo /t:GetCliBranchForTesting >> release/5.0.2xx:5.0.200-servicing.21120.4

Copy link
Member

Choose a reason for hiding this comment

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

I see.

Why should we set the placeholder value?
I'd argue we should fail if we can't fetch the correct version.

wyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should fail if something goes wrong instead of being silent about it. Please check now.

$channelMainVersion = ""
foreach($channelPart in $cli.Channel.Split('/'))
{
if($channelPart -match "\d+.*")
{
$channelMainVersion = $channelPart.Split('.')[0]
Break
}
}
if ([string]::IsNullOrEmpty($channelMainVersion)) {
Error-Log "Unable to detect channel version for dotnetinstall.ps1. The CLI install cannot be initiated." -Fatal
}

Copy link
Contributor

@heng-liu heng-liu left a comment

Choose a reason for hiding this comment

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

As we need to run dotnet integration test on Mac and Linux as well, we also install .NET SDK on Mac and Linux at https://github.com/NuGet/NuGet.Client/blob/dev/scripts/funcTests/runFuncTests.sh#L41 (version 2.2)
and https://github.com/NuGet/NuGet.Client/blob/dev/scripts/funcTests/runFuncTests.sh#L84 (version specified in config.props).
But the script we used is an old copy of dotnet-install.sh(I guess that's why we don't see the warning), since we used to have issue in downloading the dotnet-install.sh from dotnet-endpoint(see PR #3163)
Shall we consider changing for dotnet-install.sh as well? (might be in another PR if we need to do that?)
The doc of dotnet-install script shows both dotnet-install.ps1 and dotnet-install.sh have the -Quality

Copy link
Contributor

@heng-liu heng-liu left a comment

Choose a reason for hiding this comment

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

Good job! LGTM.

@erdembayar
Copy link
Contributor Author

erdembayar commented Sep 29, 2021

As we need to run dotnet integration test on Mac and Linux as well, we also install .NET SDK on Mac and Linux at https://github.com/NuGet/NuGet.Client/blob/dev/scripts/funcTests/runFuncTests.sh#L41 (version 2.2) and https://github.com/NuGet/NuGet.Client/blob/dev/scripts/funcTests/runFuncTests.sh#L84 (version specified in config.props). But the script we used is an old copy of dotnet-install.sh(I guess that's why we don't see the warning), since we used to have issue in downloading the dotnet-install.sh from dotnet-endpoint(see PR #3163) Shall we consider changing for dotnet-install.sh as well? (might be in another PR if we need to do that?) The doc of dotnet-install script shows both dotnet-install.ps1 and dotnet-install.sh have the -Quality

It looks that embedded dotnet-install.sh is another issue we need to address urgently and looks
NuGet/Home#8936 is follow up issue on it. When I compare content of embedded dotnet-install.sh and https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.sh then difference is very stark. I believe it's reason why no warning show up, even I add -Quality it's most likely not going to respect it. So it should be addressed in another PR when update content of dotnet-install.sh. I'll add comment on NuGet/Home#8936 and ping to bump up priority on it.

@zivkan
Copy link
Member

zivkan commented Oct 1, 2021

PR #4216 is downloading dotnet-install.sh on a linux machine, in the source-build job, and it works fine. So, whatever issue #3163 was mitigating is no longer needed.

I say change it back to downloading the script in this PR, and have this PR close both issues.

@erdembayar
Copy link
Contributor Author

erdembayar commented Oct 1, 2021

PR #4216 is downloading dotnet-install.sh on a linux machine, in the source-build job, and it works fine. So, whatever issue #3163 was mitigating is no longer needed.

I say change it back to downloading the script in this PR, and have this PR close both issues.

How about I merge this PR as it's and create another PR for downloading the script in another PR, because smaller the PR easier to review?

@nkolev92
Copy link
Member

nkolev92 commented Oct 1, 2021

I'd definitely say merge it while it's green, lol :D

@erdembayar
Copy link
Contributor Author

I'd definitely say merge it while it's green, lol :D

Yes, I'm merging it while it's green. I assigned the other issue to myself and changed milestone for next sprint, I'll create another PR.

@erdembayar erdembayar merged commit 966b1b2 into dev Oct 1, 2021
@erdembayar erdembayar deleted the dev-eryondon-971 branch October 1, 2021 18:23
heng-liu pushed a commit that referenced this pull request Mar 31, 2022
…igure.ps1 (#4289)

* Add -Quality Daily into dotnet-install.ps1 parameters
heng-liu pushed a commit that referenced this pull request Apr 1, 2022
…igure.ps1 (#4289)

* Add -Quality Daily into dotnet-install.ps1 parameters
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.

4 participants