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

Move to .NET 5 Preview 8 SDK #47165

Merged
merged 17 commits into from
Sep 18, 2020
Merged

Move to .NET 5 Preview 8 SDK #47165

merged 17 commits into from
Sep 18, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented Aug 26, 2020

Moves us to .NET 5 Preview 8, including the P8 SCI and SRM binaries.

@333fred 333fred requested a review from a team as a code owner August 26, 2020 21:46
@333fred
Copy link
Member Author

333fred commented Aug 26, 2020

@dotnet/roslyn-infrastructure please take a look.

I had to create a separate draft PR with this change based on master-vs-deps, as the line above the SCI/SRM binary versions was changed, causing a merge conflict. The draft is here:
#47166

I've kicked off a signed build for a test insertion:
https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4014805&view=results

Test insertion:
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/270509

@@ -243,8 +243,8 @@
-->
<NewtonsoftJsonVersion>12.0.2</NewtonsoftJsonVersion>
<StreamJsonRpcVersion>2.4.34</StreamJsonRpcVersion>
<SystemCollectionsImmutableVersion>5.0.0-preview.8.20371.14</SystemCollectionsImmutableVersion>
<SystemReflectionMetadataVersion>5.0.0-preview.8.20371.14</SystemReflectionMetadataVersion>
<SystemCollectionsImmutableVersion>5.0.0-preview.8.20407.11</SystemCollectionsImmutableVersion>
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the public versions available on NuGet.

global.json Outdated
"allowPrerelease": true,
"rollForward": "major"
},
"tools": {
"dotnet": "5.0.100-preview.7.20366.6",
"dotnet": "5.0.100-preview.8.20417.9",
"vs": {
"version": "16.6"
Copy link
Member

Choose a reason for hiding this comment

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

❔ This needs an update as well right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, probably. I just want to see the CI work first :)

@sharwell sharwell changed the title Move to P8 Move to .NET 5 Preview 8 SDK Aug 26, 2020
@jaredpar
Copy link
Member

FYI: this will mean that we need to move our minimum Visual Studio version to 16.8 preview as well. The Net5 P8 SDK depends on having 16.8 MSBuild hence in order to develop roslyn after this change you will need 16.8 preview on your machine.

I don't say this is a blocker, just as an FYI. It's been known for a while at a division level most of our repos would need to move to a preview VS for the end game of .NET5. We should tell the team though.

@@ -20,7 +20,7 @@ The minimal required version of .NET Framework is 4.7.2.
- Ensure Visual Studio is on Version "16.5" or greater
- Ensure "Use previews of the .NET Core SDK" is checked in Tools -> Options -> Environment -> Preview Features
- Restart Visual Studio
1. [.NET Core SDK 5.0 Preview 7](https://dotnet.microsoft.com/download/dotnet-core/5.0) [Windows x64 installer](https://dotnet.microsoft.com/download/dotnet/thank-you/sdk-5.0.100-preview.7-windows-x64-installer)
1. [.NET Core SDK 5.0 Preview 8](https://dotnet.microsoft.com/download/dotnet-core/5.0) [Windows x64 installer](https://dotnet.microsoft.com/download/dotnet/thank-you/sdk-5.0.100-preview.8-windows-x64-installer)
Copy link
Member

Choose a reason for hiding this comment

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

Per @jaredpar's comments, this also bumps us to requiring 16.8 -- do we need to update line 18 accordingly?

Copy link
Member

Choose a reason for hiding this comment

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

💡 If we update the solution file appropriately, earlier versions of VS will actually know to report an error if you try to open it.

MinimumVisualStudioVersion = 10.0.40219.1

@333fred
Copy link
Member Author

333fred commented Aug 27, 2020

I have validated building, testing, and F5'ing Roslyn.sln with 16.8p2. All seems to be working.

global.json Outdated Show resolved Hide resolved
@333fred
Copy link
Member Author

333fred commented Sep 2, 2020

I'm breaking the SCI and SRM updates out of this PR and into #47397 to unblock partner teams.

@333fred 333fred requested a review from a team as a code owner September 3, 2020 01:44
@333fred
Copy link
Member Author

333fred commented Sep 3, 2020

@sharwell or @JoeRobich can you please look into the integration test failures? Feel free to push to this branch.

@JoeRobich
Copy link
Member

JoeRobich commented Sep 3, 2020

hmm

image

image

@333fred
Copy link
Member Author

333fred commented Sep 9, 2020

@JoeRobich or @sharwell, any progress here?

@JoeRobich JoeRobich closed this Sep 9, 2020
@JoeRobich JoeRobich reopened this Sep 9, 2020
@333fred
Copy link
Member Author

333fred commented Sep 10, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@333fred
Copy link
Member Author

333fred commented Sep 15, 2020

I attempted to run the coreclr tests locally to figure out what is hanging. I wasn't able to see a hang locally, but I did see multiple test breakages from dotnet/runtime#42234.

@333fred 333fred requested a review from a team as a code owner September 16, 2020 23:02
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Signing off to unblock the work, but please make sure the queue change to the integration test runs are rolled back as soon as practicable.

@@ -18,22 +18,23 @@ jobs:
- job: VS_Integration
pool:
name: NetCorePublic-Pool
queue: buildpool.windows.10.amd64.vs2019.pre.open
queue: buildpool.windows.vs2019.pre.scouting.open
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this or is the Preview 2 queue now the main queue? We don't want to be permanently on the scouting queue or else we'd be get broken the moment it's updated.

Copy link
Member

Choose a reason for hiding this comment

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

(fine to merge in this state if it's necessary but please fix ASAP as otherwise this is setting a trap to break things)

Copy link
Member

Choose a reason for hiding this comment

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

Preview2 is the main queue now, which is coincidentally why the integration tests aren't quite working
https://github.com/dotnet/core-eng/wiki/VS2019-Upgrade-Schedule

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved back to the main pool.

@ghost
Copy link

ghost commented Sep 18, 2020

Hello @333fred!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@jasonmalinowski jasonmalinowski merged commit 7a36f74 into dotnet:master Sep 18, 2020
@ghost ghost added this to the Next milestone Sep 18, 2020
@333fred 333fred deleted the p8 branch September 18, 2020 21:59
Comment on lines +28 to +31
# 64-bit disabled for https://github.com/dotnet/roslyn/issues/40476
# debug_64:
# _configuration: Debug
# _oop64bit: true
Copy link
Member

Choose a reason for hiding this comment

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

😨 These are our only tests for OOP.

Copy link
Member

Choose a reason for hiding this comment

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

64-bit testing covers the 99.9% customer scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tmat and @jasonmalinowski confirmed that we were not introducing any new product bugs here, only exposing existing issues. Given that this was also blocking other work, we opted to not hold this PR hostage to existing bugs that cannot be fixed at the same time.

Copy link
Member

@jasonmalinowski jasonmalinowski Sep 20, 2020

Choose a reason for hiding this comment

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

And in parallel with that analysis, an image upgrade to our machine pool also meant the builds were entirely broken for integration tests unless we merged this PR to upgrade to Preview 8. (The SDK on the image upgraded and a version compat bug meant the mismatch broke builds.) So this quickly went from something we were doing with regret to something we had no alternative for.

Copy link
Member

@sharwell sharwell Sep 20, 2020

Choose a reason for hiding this comment

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

The integration tests were working in this pull request as of afbbec1 (10 September). I've requested this be treated as proof of a ship-blocking product regression introduced either by this change, or in parallel with this change, until we can figure the reason why they stopped working again. The fix(es) I find over the remainder of the weekend will be added to #47870.

@dibarbet dibarbet modified the milestones: Next, 16.8.P4 Sep 21, 2020
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.

8 participants