-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update to arcade 6.0 #6143
Update to arcade 6.0 #6143
Conversation
I thought the whole reason we didn't go straight to 6.0 was that we didn't want preview bits in release MSBuild? |
0948169
to
9dee02a
Compare
@Forgind that's at odds with what source-build needs, which is for us to be on arcade 6. @marcpopMSFT thoughts? arcade 6 is LTS FYI. |
The VS PR failed almost identically to https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/303587, which suggests that merging arcade 6 won't cause any issues (famous last words). We should wait until master is unblocked before merging this. |
@epinanth, is there a channel for "I want to be on and stay on arcade 6" yet? |
@epananth friendly ping (mispelled your username in my previous post). Seeing a lot of strange errors running |
Going to look at this next week, I'm on FR next week |
@epananth friendly ping 🙂 |
Looking into this one today, I will post an update soon. |
Notes to self: I was wondering why this succeeded on my local machine but was seeing "sdk not found" issues for CI builds. Turns out I had the net5 sdk installed under the .dotnet folder in the repo. Time to start updating projects to net6.0 |
We're hitting this breaking change by updating to net6.0: dotnet/docs#19625 Thread.ResetAbort() is obsolete: https://source.dot.net/#System.Private.CoreLib/Thread.cs,47d38cd18e02048c Looks like we can just ifdef around this. |
I can't seem to repro the (no bootstrap) build. Even after running Got it to repro on linux and seeing the same "could not find nuget.frameworks.dll" error. Not sure what the root cause is. Found this when digging through the assembly
|
Try merging in master? |
@Forgind Not sure what you mean? latest/master into this branch? It was updated to latest main recently. |
The PR I was thinking might have fixed it went in this morning. Doesn't look like you've picked it up yet. I've seen some issues with NuGet.Frameworks that looked very similar to that from the change to assembly loading. |
It's worth a shot! |
461dc86
to
198c784
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
e139569
to
f9a923d
Compare
Tried building from a VM with a clean VS install and running |
eng/Version.Details.xml
Outdated
@@ -14,4 +14,10 @@ | |||
<Sha>e9fd4dc7d74932c0d4b042251bc5a88bb5b3c437</Sha> | |||
</Dependency> | |||
</ToolsetDependencies> | |||
<ProductDependencies> | |||
<Dependency Name="Microsoft.Extensions.DependencyModel" Version="6.0.0-preview.2.21154.6"> |
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.
Self, is this actually a product dependency?
eng/Versions.props
Outdated
<MicrosoftNetCompilersToolsetVersion>3.9.0-2.20574.26</MicrosoftNetCompilersToolsetVersion> | ||
<NuGetBuildTasksVersion>5.9.1-rc.8</NuGetBuildTasksVersion> | ||
<NuGetBuildTasksVersion>5.10.0-preview.1.7148</NuGetBuildTasksVersion> |
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.
Shouldn't darc have been doing this for us? Are we missing a subscription?
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.
You're right: #6327 looks to be the last time this version updated automatically.
I'm thinking it was either the master->main rename or the switch from nuget.client.trusted to nuget.client that broke this.
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.
Just going to log my thought process here.
Likely not due to the switch from nuget.client.trusted since we got a few PR's since then. It's more likely that it's darc.
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.
Looks like there's no sub for it.
darc get-subscriptions --channel "VS 16.10"
https://github.com/dotnet/fsharp (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('main')
- Id: 1b3ed604-a240-43c5-f3e6-08d8e97c775d
- Update Frequency: EveryDay
- Enabled: True
- Batchable: False
- Merge Policies: []
- Last Build: 20210427.5 (b6a352ab0a11a0584348dd77811af7eecaf9d072)
https://github.com/dotnet/fsharp (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/5.0.3xx')
- Id: 8d76063a-5130-4369-8278-08d8e9754760
- Update Frequency: EveryDay
- Enabled: True
- Batchable: False
- Merge Policies:
Standard
- Last Build: 20210430.7 (e411df7595bbe379f07dd2bcbb6e4046eca9eb31)
https://github.com/dotnet/fsharp (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/6.0.1xx-preview4')
- Id: fec82357-f505-450c-c78c-08d8ff796cf5
- Update Frequency: EveryBuild
- Enabled: True
- Batchable: False
- Merge Policies:
Standard
- Last Build: 20210430.7 (e411df7595bbe379f07dd2bcbb6e4046eca9eb31)
https://github.com/dotnet/msbuild (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('main')
- Id: 9b9c2fc1-dee5-447e-827b-08d8e9754760
- Update Frequency: EveryDay
- Enabled: True
- Batchable: False
- Merge Policies:
Standard
- Last Build: 20210428.2 (5b9216a75e98e19eba84e04a5f30bd35a68f317a)
https://github.com/dotnet/msbuild (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/5.0.3xx')
- Id: 08d8569c-a921-4400-1fc7-08d8c8766580
- Update Frequency: EveryDay
- Enabled: True
- Batchable: False
- Merge Policies:
Standard
- Last Build: 20210428.2 (5b9216a75e98e19eba84e04a5f30bd35a68f317a)
https://github.com/dotnet/msbuild (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/6.0.1xx-preview4')
- Id: f3da650c-3ec0-4a87-5a84-08d8ff79a3bc
- Update Frequency: EveryBuild
- Enabled: True
- Batchable: False
- Merge Policies:
Standard
- Last Build: 20210503.2 (fa96a2a81e0fb8c028057fa204bbf386bfb36aec)
https://github.com/dotnet/roslyn (VS 16.10) ==> 'https://github.com/dotnet/arcade' ('main')
- Id: 71f51b42-1554-444f-7918-08d8e262a02c
- Update Frequency: EveryWeek
- Enabled: True
- Batchable: True
- Merge Policies:
Standard
- Last Build: 20210501.8 (4c32f5e4e9c0828a94fd4d1c9c0994082c85aaf3)
https://github.com/microsoft/vstest (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/5.0.3xx')
- Id: b4754456-bab5-48e6-093c-08d8ff7930ba
- Update Frequency: EveryBuild
- Enabled: True
- Batchable: False
- Merge Policies: []
- Last Build: 20210429-01 (1050e72d7a92a5fb5737aa6a464e8e43e7dc52f4)
https://github.com/nuget/nuget.client (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('main')
- Id: fcb199f6-ff33-44a0-f3ef-08d8e97c775d
- Update Frequency: EveryBuild
- Enabled: True
- Batchable: False
- Merge Policies: []
- Last Build: 5.10.0.7228 (d2bb0de35242b04802c1a7856ebb7718f674565c)
https://github.com/nuget/nuget.client (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/5.0.3xx')
- Id: 61b07feb-68a2-4b3c-64a4-08d8e9757c68
- Update Frequency: EveryBuild
- Enabled: True
- Batchable: False
- Merge Policies: []
- Last Build: 5.10.0.7228 (d2bb0de35242b04802c1a7856ebb7718f674565c)
https://github.com/nuget/nuget.client (VS 16.10) ==> 'https://github.com/dotnet/sdk' ('release/6.0.1xx-preview4')
- Id: 5184e9dc-a7d8-43a0-c78a-08d8ff796cf5
- Update Frequency: EveryBuild
- Enabled: True
- Batchable: False
- Merge Policies:
Standard
- Last Build: 5.10.0.7240 (dca1d060f38e1e02f6bfca41e25f081f19fd534b)
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.
Correctomundo! darc add-subscription --channel "VS 16.10" --source-repo "https://github.com/nuget/nuget.client" --target-repo "https://github.com/dotnet/msbuild" --target-branch main --update-frequency "everyWeek" --trigger
See #6405
@@ -0,0 +1,2158 @@ | |||
// Copyright (c) Microsoft. All rights reserved. |
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.
Something's up here; this shouldn't be a new file.
src/Directory.BeforeCommon.targets
Outdated
@@ -122,9 +122,10 @@ | |||
<DefineConstants>$(DefineConstants);FEATURE_RUNTIMEINFORMATION</DefineConstants> | |||
<DefineConstants>$(DefineConstants);USE_MSBUILD_DLL_EXTN</DefineConstants> | |||
<DefineConstants>$(DefineConstants);WORKAROUND_COREFX_19110</DefineConstants> | |||
<DefineConstants Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))">$(DefineConstants);NET6_PLUS</DefineConstants> |
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.
IIRC the SDK defines something much like this. Can we use that instead?
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.
running rg -Fi "<defineconstants" -tmsbuild
in the sdk doesn't look promising. Sounds like something we can merge into the SDK to allow compiler directives in code for breaking changes between sdk versions. Something like:
<DefineConstants Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netcoreapp3.1'))">$(DefineConstants);NET31_PLUS</DefineConstants>
<DefineConstants Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net5.0'))">$(DefineConstants);NET5_PLUS</DefineConstants>
<DefineConstants Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))">$(DefineConstants);NET6_PLUS</DefineConstants>
@dsplaisted does this sound reasonable?
Questions answered with some greppin'
- Where is DefineConstants even consumed?
- Looks like it's passed as an argument to the compiler. https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.CSharp.CurrentVersion.targets#L249
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.
dotnet/sdk#14798 added it; in this case it's NET6_0_OR_GREATER
.
a8bb518
to
a3dd38b
Compare
* darc update-dependencies --source-repo arcade --channel '.NET Eng - Latest' * update DotNetCliVersion to match global.json
Because we use binaries from the initial .NET SDK to build up our bootstrap directory, we're tied to the runtime version of that SDK. Since we will target .NET 6 for 17.0, this is a reasonable move now. * Keep ref file for .NET 6 in netstandard * This isn't strictly correct but is where we have been keeping ".NET Core" API surface. * Use NET6_0_OR_GREATER preprocessor symbol * This API is no longer available in .NET 6. * Update bootstrap build scripts to look for net6.0 binaries * Update Microsoft.Extensions.DependencyModel * This needs to roughly match the SDK that we're assembling in the bootstrap folder.
This should fix CI builds running on older VS images.
a3dd38b
to
29e17e9
Compare
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.
LGTM pending #6408.
</Dependency> | ||
<Dependency Name="NuGet.Build.Tasks" Version="5.10.0-rc.7240"> | ||
<Uri>https://github.com/nuget/nuget.client</Uri> | ||
<Sha>dca1d060f38e1e02f6bfca41e25f081f19fd534b</Sha> | ||
</Dependency> | ||
<Dependency Name="Microsoft.Extensions.DependencyModel" Version="6.0.0-preview.2.21154.6"> |
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.
Do we own this/should we be specifying its version? Do we have a plan for keeping it up-to-date with the version specified in Versions.props?
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.
yes: darc subscription. Can't set it up until this merges though.
@MichaelSimons Not yet but Soon™. We have finally gotten unblocked on checking in changes to VS and are working through backlog. This is high on the priority list. |
Fixes #6119
Context
This is to get MSBuild on arcade powered source-build.
Changes Made
Ran
darc update-dependencies --source-repo arcade --channel ".NET 5 Eng - Latest"
3/3: Ran
darc update-dependencies --source-repo arcade --channel ".NET Eng - Latest"
Testing
CI is enough.
Notes
I expect this PR to be in flight as I chip away at it over time. Or it will magically work first time through.
For reviewers: The first commit is the biggest diff and was entirely generated by running the darc command above, so you may want to start at commit 2 when it comes up.