Skip to content
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

Replace KoreBuild with Arcade #11122

Merged
merged 77 commits into from Jun 19, 2019

Conversation

@natemcmaster
Copy link
Member

commented Jun 12, 2019

Part of #7280

Resolves dotnet/arcade#88

High level changes

  • Update build.sh/ps1 to call into Arcade instead of KoreBuild. This completely removes our usage of KoreBuild
  • Move all contents in build/ (the KoreBuild convention) into their matching version in eng/
  • Remove dead code and other features which were KoreBuild only
    • Remove usage of NuGetPackageVerifier
    • Remove signcheck
    • Remove AddAllProjectRefsToSolution.ps1 (Resolves #7740 )

TODO (before this PR is ready to merge)

  • Get tests running
  • Ensure we get the same build output
  • Ensure code gen steps still work
  • Consume a new update of Arcade which has necessary fixes

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

natemcmaster added some commits Jun 8, 2019

Show resolved Hide resolved build.ps1

@JunTaoLuo JunTaoLuo self-requested a review Jun 12, 2019

JunTaoLuo and others added some commits Jun 12, 2019

Fixups to broken build
* capture test results in xunit form
* attempt to fix code check
* restore before linux build
* remove duplicate signinfos
@natemcmaster

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

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?

@JunTaoLuo

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Sure let's separate that work out of this PR. Let's discuss what the issues are.

natemcmaster added some commits Jun 13, 2019

More build fixes
* exclude node_modules from unique project check
* fixup signing props
More build fixes
* Remove unused NoWarns
* Skip building site extension
* Suppress xunit color in console output
* Install x86 runtime
@JunTaoLuo

This comment has been minimized.

@aspnet-hello

This comment has been minimized.

Copy link

commented Jun 18, 2019

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)
aspnet/AspNetCore-Internal#2623
aspnet/AspNetCore-Internal#2410
aspnet/AspNetCore-Internal#2415

@JunTaoLuo

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Tests are passing! Just need to verify build output before merging.

Show resolved Hide resolved Directory.Build.props
@JunTaoLuo

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Just missing a signoff from @dougbu and another run of the tests to see if there are any flakiness.

@natemcmaster

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

image

Nice work! Thanks for following through on this John!

@dougbu

dougbu approved these changes Jun 19, 2019

Copy link
Member

left a comment

Generally looks good (though I'm not sure I really got everything in this huge PR)

-arch x86
-NoRestore

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 19, 2019

Member

Why is this gone? Same for a few other -NoRestore and --no-restore removals.

This comment has been minimized.

Copy link
@JunTaoLuo

JunTaoLuo Jun 19, 2019

Contributor

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.

Show resolved Hide resolved .azure/pipelines/helix-test.yml Outdated
<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" />

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 19, 2019

Member

Are these new references to projects not mentioned elsewhere showing up due to merges from 'master'?

This comment has been minimized.

Copy link
@JunTaoLuo

JunTaoLuo Jun 19, 2019

Contributor

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());

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 19, 2019

Member

Lots 'o C# changes from here on also look odd. Are these also due to merges from 'master'?

This comment has been minimized.

Copy link
@JunTaoLuo

JunTaoLuo Jun 19, 2019

Contributor

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 aspnet/Extensions#1363?

This comment has been minimized.

Copy link
@Tratcher

Tratcher Jun 19, 2019

Member

Correct

@JunTaoLuo

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Followup items:

@jkotalik

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@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.

@dougbu

This comment has been minimized.

@aspnet-hello

This comment has been minimized.

Copy link

commented Jun 19, 2019

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)
aspnet/AspNetCore-Internal#2489

@dougbu

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Helix completed, not sure why the PR check isn't showing green/red.

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.

@JunTaoLuo JunTaoLuo merged commit 4fde84a into master Jun 19, 2019

14 of 16 checks passed

AspNetCore-ci
Details
AspNetCore-helix-test
Details
AspNetCore-ci (Build: Linux ARM) Build: Linux ARM succeeded
Details
AspNetCore-ci (Build: Linux ARM64) Build: Linux ARM64 succeeded
Details
AspNetCore-ci (Build: Linux Musl ARM64) Build: Linux Musl ARM64 succeeded
Details
AspNetCore-ci (Build: Linux Musl x64) Build: Linux Musl x64 succeeded
Details
AspNetCore-ci (Build: Linux x64) Build: Linux x64 succeeded
Details
AspNetCore-ci (Build: Windows ARM) Build: Windows ARM succeeded
Details
AspNetCore-ci (Build: Windows x64/x86) Build: Windows x64/x86 succeeded
Details
AspNetCore-ci (Build: macOS) Build: macOS succeeded
Details
AspNetCore-ci (Code check) Code check succeeded
Details
AspNetCore-ci (Test: Templates - Windows Server 2016 x64) Test: Templates - Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: Ubuntu 16.04 x64) Test: Ubuntu 16.04 x64 succeeded with issues
Details
AspNetCore-ci (Test: Windows Server 2016 x64) Test: Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: macOS 10.13) Test: macOS 10.13 succeeded with issues
Details
license/cla All CLA requirements met.
Details

@aspnet-github-bot aspnet-github-bot bot deleted the namc/arcade branch Jun 19, 2019

javiercn added a commit that referenced this pull request Jun 20, 2019

Revert "Replace KoreBuild with Arcade (#11122)"
This reverts commit 4fde84a.

This is done to unblock myself. Will remove before merging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.