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

Upgrade build SDK to 5.0 preview8 #22227

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Upgrade build SDK to 5.0 preview8 #22227

merged 1 commit into from
Aug 26, 2020

Conversation

smitpatel
Copy link
Member

No description provided.

@smitpatel smitpatel marked this pull request as ready for review August 26, 2020 00:17
@smitpatel smitpatel requested a review from a team August 26, 2020 00:17
@smitpatel
Copy link
Member Author

@Pilchie for approval.

@@ -22,6 +22,7 @@
<ItemGroup>
<PackageReference Include="StyleCop.Analyzers" Version="1.1.118" PrivateAssets="All" />
<PackageReference Include="Microsoft.Azure.Cosmos" Version="3.12.0" />
<PackageReference Include="Microsoft.Azure.Cosmos.Direct" Version="3.11.4" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #22231

Copy link
Member

Choose a reason for hiding this comment

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

Does it compile from commandline without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No 😞

@@ -0,0 +1,10 @@
<Project>
Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #22230

@@ -33,30 +31,5 @@ public void OperationException_exposes_public_string_and_inner_exception_constru
Assert.Equal("Foo", ex.Message);
Assert.Same(inner, ex.InnerException);
}

[ConditionalFact]
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 tried using "recommended approach" as per release notes and JSON serializer. It did not work since we are create ad-hoc exception (rather than actual one being thrown which would have stack trace and other data).
If we should have these then I can file an issue to add them again.

Copy link
Member

Choose a reason for hiding this comment

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

File an issue to discuss

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine to remove them. It will also stop us being noise when people search for code using BinaryFormatter. 🙃

Copy link
Member Author

Choose a reason for hiding this comment

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

global.json Outdated
Comment on lines 4 to 6
"runtimes": {
"dotnet": [
"3.1.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

Upgrade to 3.1.7 and remove aspnetcore

Copy link
Member Author

Choose a reason for hiding this comment

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

Design/SqlServer.Tests/CrossStore failed without aspnetcore on netcoreapp3.1

Copy link
Contributor

Choose a reason for hiding this comment

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

☹ One of these days...

Copy link
Member

Choose a reason for hiding this comment

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

Just curious, what was the error here? Why do we still need aspnetcore?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not possible to find any compatible framework version
The framework 'Microsoft.AspNetCore.App', version '3.1.0' was not found.
  - The following frameworks were found:
      5.0.0-preview.8.20414.8 at [F:\workspace\_work\1\s\.dotnet\shared\Microsoft.AspNetCore.App]

You can resolve the problem by installing the specified framework and/or SDK.

The specified framework can be found at:
  - https://aka.ms/dotnet-core-applaunch?framework=Microsoft.AspNetCore.App&framework_version=3.1.0&arch=x64&rid=win10-x64
=== COMMAND LINE ===
"F:\workspace\_work\1\s\.dotnet\dotnet.exe" exec --depsfile "F:\workspace\_work\1\s\artifacts\bin\EFCore.SqlServer.Tests\Release\netcoreapp3.1\Microsoft.EntityFrameworkCore.SqlServer.Tests.deps.json" --runtimeconfig "F:\workspace\_work\1\s\artifacts\bin\EFCore.SqlServer.Tests\Release\netcoreapp3.1\Microsoft.EntityFrameworkCore.SqlServer.Tests.runtimeconfig.json"  "F:\workspace\_work\1\s\.packages\xunit.runner.console/2.4.1/tools/netcoreapp2.0/xunit.console.dll" "F:\workspace\_work\1\s\artifacts\bin\EFCore.SqlServer.Tests\Release\netcoreapp3.1\Microsoft.EntityFrameworkCore.SqlServer.Tests.dll" -noautoreporters -xml "F:\workspace\_work\1\s\artifacts\TestResults\Release\EFCore.SqlServer.Tests_netcoreapp3.1_x64.xml" -html "F:\workspace\_work\1\s\artifacts\TestResults\Release\EFCore.SqlServer.Tests_netcoreapp3.1_x64.html"  > "F:\workspace\_work\1\s\artifacts\log\Release\EFCore.SqlServer.Tests_netcoreapp3.1_x64.log" 2>&1

I tried to do cursory analysis, what are we using but log did not indicate anything specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

The SDK eagerly requires Microsoft.AspNetCore.App. There are workarounds to avoid it in other repos. I think a fix went in for it recently...

Copy link
Contributor

@bricelam bricelam Aug 26, 2020

Choose a reason for hiding this comment

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

@smitpatel smitpatel force-pushed the smit/preview8SDK branch 2 times, most recently from 5dbb3c4 to cb963b3 Compare August 26, 2020 15:30
@Pilchie
Copy link
Member

Pilchie commented Aug 26, 2020

Approved for RC1.

@smitpatel smitpatel merged commit aca0037 into release/5.0 Aug 26, 2020
@smitpatel smitpatel deleted the smit/preview8SDK branch August 26, 2020 18:47
@mmitche
Copy link
Member

mmitche commented Aug 26, 2020

This appears to have broken the signing validation stages in the official build: https://dev.azure.com/dnceng/internal/_build/results?buildId=789168&view=logs&j=0a6c679f-72be-5867-422e-acb741a068a3&t=d8e44ddf-8ea2-5518-25e7-4300471a271a

This is because the hosted pool machines don't have 16.8 These signing validations still exist in the official build of efcore because this repo won't be on the critical path post-build, and so the validation stages were allowed to remain for convenience (same goes for tests, as none of the other repos run tests in their official builds).

Solutions for this:

  • Onboard to post-build validation for this repo (fairly simple, removes these steps from the official build)
  • Revert this change (do we need to build with p8 for rc1?)
  • Temporarily tweak the validation jobs to use a preview queue (I think I'd make this in the repo only, rather than rolling it out to eveyrone using arcade

/cc @dotnet/dnceng @markwilkie

@ajcvickers
Copy link
Member

This is because the hosted pool machines don't have 16.8

We're telling customers they need 16.8 for .NET 5 preview 8. Shouldn't we be building/testing with it?

Revert this change (do we need to build with p8 for rc1?)

We found several issues moving to preview 8 that we now need to investigate. Delaying this change further doesn't seem wise since customers already have preview 8 in their hands.

Onboard to post-build validation for this repo

If this means switching to not testing what we actually ship, then I'm not in favor. But the only way we can ship something is if we don't test that thing, then so be it.

Temporarily tweak the validation jobs to use a preview queue

Daily builds are the most important to us. As long as we are still getting daily builds of main, then this is fine.

@mmitche
Copy link
Member

mmitche commented Aug 26, 2020

I think it's worth keeping preview8 here.

This is not about anything but the validation jobs, which run after the build has completed (prior to publishing). They're really just running signtool and a couple other random tools (like policheck). I don't think that running these on 16.8 has any added value.

Btw, EFCore is actually using VS2017 machines for its build. I think this only hits because some of the validation stages need desktop msbuild (e.g. when they use microbuild to do a signing check)

Daily builds are the most important to us. As long as we are still getting daily builds of main, then this is fine.

Right now, the version of efcore in RC 1 is basically blocked from updating. main appears to still be fine.

@smitpatel
Copy link
Member Author

Right now, the version of efcore in RC 1 is basically blocked from updating. main appears to still be fine.

Only till release/5.0 is not merged to main. Which @ajcvickers does every morning I believe.

@mmitche
Copy link
Member

mmitche commented Aug 27, 2020

Do you all have a preference for what we do here? I am thinking to onboard to the post-build validation stages is the best path.

@riarenas
Copy link
Member

No validation is lost by doing that, and also has the advantage of being the easiest fix. I also think it's the best path here.

@Pilchie
Copy link
Member

Pilchie commented Aug 27, 2020

I tend to agree.

@ajcvickers
Copy link
Member

Assuming I understand correctly, then I'm fine with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants