-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Revert disabling the toolset package in Helix #42860
Conversation
ae9a812
to
05a9ef0
Compare
05a9ef0
to
c5e8a24
Compare
41789e6
to
b0acfed
Compare
017efb9
to
b0acfed
Compare
@marcpopMSFT @jaredpar @baronfel for reviews, thanks |
@@ -232,6 +232,7 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
<PropertyGroup Condition="'$(BuildWithNetFrameworkHostedCompiler)' == 'true' and '$(OS)' == 'Windows_NT'"> | |||
<RoslynTargetsPath>$(NuGetPackageRoot)\microsoft.net.sdk.compilers.toolset\$(NETCoreSdkVersion)</RoslynTargetsPath> | |||
<_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage>true</_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage> | |||
<_SkipCheckMicrosoftNetSdkCompilersToolsetPackageExists Condition="'$(NuGetPackageRoot)' == ''">true</_SkipCheckMicrosoftNetSdkCompilersToolsetPackageExists> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment/link to the issues/discussions that triggered this check? I expect future-us to forget and having some record of why this was necessary would be helpful.
@@ -38,6 +37,7 @@ REM We downloaded a special zip of files to the .nuget folder so add that as a s | |||
dotnet nuget list source --configfile %TestExecutionDirectory%\nuget.config | |||
PowerShell -ExecutionPolicy ByPass "dotnet nuget locals all -l | ForEach-Object { $_.Split(' ')[1]} | Where-Object{$_ -like '*cache'} | Get-ChildItem -Recurse -File -Filter '*.dat' | Measure" | |||
dotnet nuget add source %DOTNET_ROOT%\.nuget --configfile %TestExecutionDirectory%\nuget.config | |||
if exist %TestExecutionDirectory%\Testpackages dotnet nuget add source %TestExecutionDirectory%\Testpackages --name testpackages --configfile %TestExecutionDirectory%\nuget.config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the script work as is for local helix execution or are changes needed to the instructions here: https://github.com/dotnet/sdk/blob/main/documentation/project-docs/repro-helix-failure.md
We don't do this often but that means it's fragile to changes if we're not careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know about this doc, thanks.
I've followed the instructions and they seem to work, in particular the nuget source is added correctly:
C:\temp\helix\payload-dir>if exist C:\Users\janjones\AppData\Local\Temp\dotnetSdkTests\jxi5tacr.5hi\Testpackages dotnet nuget add source C:\Users\janjones\AppData\Local\Temp\dotnetSdkTests\jxi5tacr.5hi\Testpackages --name testpackages --configfile C:\Users\janjones\AppData\Local\Temp\dotnetSdkTests\jxi5tacr.5hi\nuget.config
Package source with Name: testpackages added successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, this PR is targeting branch release/9.0.1xx
, let me know if that's not correct
Gonna let Marc see your comment about the Helix stuff, but then this LGTM. This branch is great for the GA release - we'll want to do a backport to main once it merges. |
@marcpopMSFT if this looks good, feel free to merge, thanks |
Looks like the local helix repro steps work with this change so we should be good here (that was my main concern). |
Resolves #42845.