-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Replace KoreBuild with Arcade #11122
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
Conversation
…rSolutionBuild.targets
af61c9d
to
31563be
Compare
* capture test results in xunit form * attempt to fix code check * restore before linux build * remove duplicate signinfos
Hmm, SiteExtensions are super broken by this. I'm looking at the targets which implement the site extension build and big chunks will probably need to be rewritten to work with Arcade. @JunTaoLuo would you be okay temporarily disabling the site extension projects and doing that separately? |
Sure let's separate that work out of this PR. Let's discuss what the issues are. |
* exclude node_modules from unique project check * fixup signing props
* Remove unused NoWarns * Skip building site extension * Suppress xunit color in console output * Install x86 runtime
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
Tests are passing! Just need to verify build output before merging. |
src/ProjectTemplates/Web.Spa.ProjectTemplates/Microsoft.DotNet.Web.Spa.ProjectTemplates.csproj
Outdated
Show resolved
Hide resolved
Just missing a signoff from @dougbu and another run of the tests to see if there are any flakiness. |
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.
Generally looks good (though I'm not sure I really got everything in this huge PR)
-arch x86 | ||
-NoRestore |
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.
Why is this gone? Same for a few other -NoRestore
and --no-restore
removals.
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.
Hmm I might want to investigate this separately. Specifically because I know there were some changes with lifetimes of targets and interleaving of steps introduced with converting to arcade so these changes may be deliberate. Since I wouldn't want to hold up the PR and this is more of an optimization, I would do this as a follow up.
<ProjectReference Include="$(RepoRoot)src\DefaultBuilder\testassets\DependencyInjectionApp\DependencyInjectionApp.csproj" ReferenceOutputAssemblies="false" /> | ||
<ProjectReference Include="$(RepoRoot)src\DefaultBuilder\testassets\StartRequestDelegateUrlApp\StartRequestDelegateUrlApp.csproj" ReferenceOutputAssemblies="false" /> | ||
<ProjectReference Include="$(RepoRoot)src\DefaultBuilder\testassets\StartRouteBuilderUrlApp\StartRouteBuilderUrlApp.csproj" ReferenceOutputAssemblies="false" /> | ||
<ProjectReference Include="$(RepoRoot)src\DefaultBuilder\testassets\StartWithIApplicationBuilderUrlApp\StartWithIApplicationBuilderUrlApp.csproj" ReferenceOutputAssemblies="false" /> |
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.
Are these new references to projects not mentioned elsewhere showing up due to merges from 'master'?
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.
I added these references to ensure build order between test assets and functional tests.
@@ -173,7 +173,8 @@ public void ConfigureDefaultServiceProvider(IWebHostBuilder builder) | |||
options.ValidateScopes = true; | |||
}); | |||
|
|||
Assert.Throws<InvalidOperationException>(() => hostBuilder.Build().Start()); | |||
using var host = hostBuilder.Build(); | |||
Assert.Throws<InvalidOperationException>(() => host.Start()); |
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.
Lots 'o C# changes from here on also look odd. Are these also due to merges from 'master'?
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.
No this is to prevent a hang. Not disposing generic host builders caused this so I had to add using in several places. @Tratcher is this the issue tacking that bug dotnet/extensions#1363?
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.
Correct
If the next round of tests succeed, I'll conclude that these changes have not introduced any additional flakiness (a total of 3 successes) in our builds and will merge. I'll also file follow up issues. |
@JunTaoLuo I'll also make a follow-up PR to fix potential native dll copying issues. Let's get this in! Helix completed, not sure why the PR check isn't showing green/red. |
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
We're still seeing the occasional missed messages between AzDO and GitHub. Since the Helix failure is of a known-flaky test, I suggest we get this in. @JunTaoLuo please do the honours. |
Part of #7280
Resolves dotnet/arcade#88
High level changes
TODO (before this PR is ready to merge)
TODO (will be done in a separate PR)
* This removed support for generating Maestro manifests during build. We'll need to re-enable this somehow.handled in this PR by keeping existing Maestro manifest generation