Skip to content

Replace Internal.AspNetCore.Sdk with Microsoft.DotNet.Arcade.Sdk #10674

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

Merged
merged 45 commits into from
Jun 8, 2019

Conversation

natemcmaster
Copy link
Contributor

This is part of #7280.

Here are the high-level changes:

  • Change location of dotnet.exe to $repoRoot/dotnet.exe instead of $repoRoot/x64/dotnet.exe
  • Remove our own implementation of versioning properties and targets
  • Remove licenseUrl from all nuget packages and use PackageLicenseExpression instead
  • Remove a bunch of properties and targets from our code that are provided by Microsoft.DotNet.Arcade.Sdk (or are unnecessary now)
  • Add a handful of workaround to override defaults from Arcade which don't work for this repo

Work remaining to complete #7280 (will be done in a separate PR)

  • Replace KoreBuild usages in build scripts with Arcade scripts
  • Update Azure pipelines to use Arcade YAML templates

FYI - marking this as a draft PR until I can get CI to pass. Local builds are working, but still need to validate CI output and tests don't change substantially.

/cc @Pilchie @markwilkie @tmat @aspnet/build

Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

Can't say I know KoreBuild well enough, or reviewed this closely enough to be super confident, but glad to see it happening!

@natemcmaster
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@natemcmaster
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@natemcmaster natemcmaster requested review from Tratcher and a team as code owners June 6, 2019 06:20
@analogrelay
Copy link
Contributor

Farewell (almost) KoreBuild, you served us well :).

Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Mostly in the same boat as @Pilchie, a lot of this is beyond what I was familiar with. But glad to see us get closer in alignment with Arcade :). Good to do this early in preview 7 too so there's some time to shake out the bugs.

Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Mostly in the same boat as @Pilchie, a lot of this is beyond what I was familiar with. But glad to see us get closer in alignment with Arcade :). Good to do this early in preview 7 too so there's some time to shake out the bugs.

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

I didn't spot any problems, but this is a big one with a lot of moving parts, so I feel like the proof will mostly be in if it works or not.

@@ -348,6 +348,10 @@ if ($RunBuild -and ($All -or $BuildJava) -and -not $NoBuildJava) {
}
}

if ($env:PATH -notlike "*${env:JAVA_HOME}*") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify/comment on what this is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RepoRoot=$(AzureIntegrationProjectRoot);
VersionSuffix=$(VersionSuffix);
BuildNumberSuffix=$(BuildNumberSuffix);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these properties still available to the Azure integration projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VersionSuffix should be - that's a built in SDK property. BuildNumberSuffix is being removed, but I didn't see it getting used anywhere in Azure integration.

@natemcmaster
Copy link
Contributor Author

I didn't spot any problems, but this is a big one with a lot of moving parts, so I feel like the proof will mostly be in if it works or not.

Agreed. I'm doing a thorough diff of the build outputs to make sure things like nuspecs, manifests, filenames, etc. didn't change unexpectedly.

@ryanbrandenburg - I'm getting consistent test failures on non-Windows platforms in the templates. I couldn't figure out how my changes would cause this failure. Are test failures here a known issue?
https://dev.azure.com/dnceng/public/_build/results?buildId=214984&view=ms.vss-test-web.build-test-results-tab&runId=5269634&resultId=110652&paneView=debug
image

@natemcmaster natemcmaster added the blocked The work on this issue is blocked due to some dependency label Jun 6, 2019
@natemcmaster
Copy link
Contributor Author

Blocked on dotnet/arcade#2984 - haven't found a workaround. Struggling to figure out to make our .wixproj files work without some nasty hacks. Evaluation order for VersionSuffix moved from Directory.Build.props to Microsoft.Common.targets, which makes it hard to use $(PackageVersion) in the output file name for a Windows installer.

@ryanbrandenburg
Copy link
Contributor

@natemcmaster The templating link failures should be fixed by #10957 if you rebase on to latest.

@natemcmaster
Copy link
Contributor Author

Thanks Ryan! I'll update and try again

@natemcmaster natemcmaster removed the blocked The work on this issue is blocked due to some dependency label Jun 7, 2019
@natemcmaster
Copy link
Contributor Author

I think I found a workaround for dotnet/arcade#2984 and have pushed a fix to the .wixproj files.

@natemcmaster
Copy link
Contributor Author

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2646

@natemcmaster
Copy link
Contributor Author

This is a big PR. I've carefully looked over the build outputs and don't see any major unexpected changes. Merging now so we can move forward with the next round of Arcade improvements.

@natemcmaster natemcmaster merged commit 41ce223 into master Jun 8, 2019
@ghost ghost deleted the namc/arcade branch June 8, 2019 00:19
@analogrelay analogrelay added this to the 3.0.0-preview7 milestone Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants