Skip to content

Update SDK version #33719

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 25, 2021
Merged

Update SDK version #33719

merged 6 commits into from
Jun 25, 2021

Conversation

BrennanConroy
Copy link
Member

Part of build ops

@BrennanConroy BrennanConroy requested a review from a team as a code owner June 21, 2021 17:58
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 21, 2021
@BrennanConroy BrennanConroy requested a review from Pilchie as a code owner June 21, 2021 20:00
dougbu
dougbu reviewed Jun 21, 2021
View reviewed changes
src/ProjectTemplates/Shared/TemplatePackageInstaller.cs
var proc = await RunDotNetNew(output, $"");
if (!proc.Output.Contains($" {templateName} "))
var proc = await RunDotNetNew(output, $"--list");
if (!(proc.Output.Contains($" {templateName} ") || proc.Output.Contains($",{templateName}") || proc.Output.Contains($"{templateName},")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you quickly summarize why these lines in the installer need to change❔ For example has a bare dotnet new command been deprecated at the same time the output format changed❔

Copy link
Member Author

Choose a reason for hiding this comment

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

For example has a bare dotnet new command been deprecated at the same time the output format changed❔

No clue, I'm testing to see if that's the case by using --list.

The first part with checking commas is because dotnet new now displays all "short names" for the template. Might be able to clean it up a bit, but again, doing exploratory testing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. I don't remember a breaking change notification but the output might not be something we should really rely on. @KathleenDollard who should we loop in here❔

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like --list worked, now just failing on some restore issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Old dotnet new output would list all templates it looks like, new dotnet new lists a predetermined list of "common templates".

Looks like a very deliberate change, so I think our reaction to add --list is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the ors I added can be removed now that --list is used

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, I checked and it was displaying templates the way your are testing them. For me the --list would just display a new prompt, and then the same table as before without the --list.

@sebastienros
Copy link
Member

sebastienros commented Jun 24, 2021 •
edited
Loading

Build is failing on mac and linux while restoring nuget packages for two tests:

[xUnit.net 00:05:13.78]     Templates.Test.EmptyWebTemplateTest.EmptyWebTemplateCSharp [FAIL]

 | [11.662s] Templates.Test.EmptyWebTemplateTest Information: The template "ASP.NET Core Empty" was created successfully.
 | [11.673s] Templates.Test.EmptyWebTemplateTest Information: 
 | [11.673s] Templates.Test.EmptyWebTemplateTest Information: Processing post-creation actions...
 | [11.674s] Templates.Test.EmptyWebTemplateTest Information: Running 'dotnet restore' on /datadisks/disk1/work/9C2008E7/w/9F50091A/e/Templates/BaseFolder/AspNet.4jel0wrqdam/AspNet.4jel0wrqdam.csproj...
 | [311.903s] Templates.Test.EmptyWebTemplateTest Information: [ERROR] Restore failed.

[xUnit.net 00:11:59.26]     Templates.Test.WebApiTemplateTest.WebApiTemplateCSharp_IdentityWeb_BuildsAndPublishes(auth: "SingleOrg", args: null) [FAIL]

 | [27.729s] Templates.Test.WebApiTemplateTest Information: The template "ASP.NET Core Web API" was created successfully.
 | [27.729s] Templates.Test.WebApiTemplateTest Information: 
 | [27.729s] Templates.Test.WebApiTemplateTest Information: Processing post-creation actions...
 | [27.730s] Templates.Test.WebApiTemplateTest Information: Running 'dotnet restore' on /datadisks/disk1/work/9C2008E7/w/9F50091A/e/Templates/BaseFolder/AspNet.kthak30aajf/AspNet.kthak30aajf.csproj...
 | [328.501s] Templates.Test.WebApiTemplateTest Information: [ERROR] Restore failed.

It looks like restore is interrupted after 5 minutes, but there is no log about the reason.

The SDK code that executes this step is

https://github.com/dotnet/templating/blob/main/src/Microsoft.TemplateEngine.Cli/PostActionProcessors/DotnetRestorePostActionProcessor.cs#L92-L93

When run locally, the ProjectTemplates tests seem blocked, so the CI might actually have some sort of timeout that interrupts them.

@dougbu
Copy link
Contributor

dougbu commented Jun 25, 2021

It looks like restore is interrupted after 5 minutes, but there is no log about the reason.

Not sure where 5 minutes comes from.

The most common timeout in template tests is 15 minutes for various dotnet processes. See ProcessEx changes in #32889 for something that changes that value. There's also a ProcessLock class used to wait for process completion. The wait defaults to 20 minutes and is never overridden.

In addition, the dotnet test command run in Helix jobs (see

aspnetcore/eng/helix/content/RunTests/TestRunner.cs

Line 218 in 46ef939

var commonTestArgs = $"test {Options.Target} --diag:{diagLog} --logger:xunit --logger:\"console;verbosity=normal\" --blame \"CollectHangDump;TestTimeout=15m\"";
) specifies a fifteen minute test timeout (for dump collection). In addition,

aspnetcore/eng/helix/content/RunTests/TestRunner.cs

Lines 215 to 216 in 46ef939

// Timeout test run 5 minutes before the Helix job would timeout
var cts = new CancellationTokenSource(Options.Timeout.Subtract(TimeSpan.FromMinutes(5)));
times tests out five minutes before the Helix job times out. The default Helix timeout is 30 minutes, 40 on Windows (see

aspnetcore/eng/targets/Helix.props

Lines 14 to 15 in 0551cef

<HelixTimeout>00:30:00</HelixTimeout>
<HelixTimeout Condition="$(HelixTargetQueue.StartsWith('Windows.10.Amd64'))">00:40:00</HelixTimeout>
).

@BrennanConroy BrennanConroy mentioned this pull request Jun 25, 2021
Merged
@sebastienros
Copy link
Member

I think it's a timeout in dotnet restore itself, or in the test runner, I can repro the same time locally when running a single test.

@sebastienros
Copy link
Member

Actually found the timeout in ProcessEx https://github.com/dotnet/aspnetcore/blob/main/src/Shared/Process/ProcessEx.cs#L31

This could mean that dotnet restore is taking more than 5 minutes and the ProcessEx class is killing it.

@sebastienros sebastienros merged commit e03e116 into main Jun 25, 2021
@sebastienros sebastienros deleted the brecon/sdk branch June 25, 2021 18:24
@ghost ghost added this to the 6.0-preview7 milestone Jun 25, 2021
halter73 added a commit that referenced this pull request Jun 28, 2021
@halter73
Revert "Update SDK version (#33719)"
1edb33b 
This reverts commit e03e116.
@dougbu dougbu mentioned this pull request Jun 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastienros sebastienros sebastienros left review comments

@Pilchie Pilchie Awaiting requested review from Pilchie

+1 more reviewer

@dougbu dougbu dougbu approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
6.0-preview7
Development

Successfully merging this pull request may close these issues.
3 participants
@BrennanConroy @sebastienros @dougbu