MSBuild Upgrade: netstandard2.0 -> net6.0 #6148
Conversation
|
@Forgind do you recognize the issue with |
|
Just looking at the list of Windows Core errors, there were a variety of different problems here, only two of which are related to RAR. If you can get a diagnostic build log or binlog, I can probably find where conflict is coming from—it looks like some assemblies refer to a specific version (4.0.0.0) of System.Configuration, whereas some other refers to it without a version, and RAR was confused. Unfortunately, only one binlog (Windows Full) output a binlog, as far as I can tell, and that one didn't have that problem, so I don't have much else to go on. Are you seeing this problem locally? If so, I can try pulling it down and looking into it. |
|
@Forgind I think it was due to multiple |
benvillalobos
force-pushed
the
netstandard2.0-to-net5.0
branch
from
85c453e
to
a389596
Compare
|
@ladipro running Note that this PR updated in Microsoft.NET.StringTools.Tests.WeakStringCache_Tests.RetainsLastStringWithGivenHashCode and Microsoft.NET.StringTools.Tests.WeakStringCache_Tests.RetainsStringUntilCollected Any idea why going from netstandard2.0 to net5.0 would break these specific tests? Also please double check commit bc7c4e1, I had to relax some nullable warnings. |
|
@dsplaisted could you take a look at 8bbdc98? I'm not sure how to go adding these ref assemblies, or if we even need them anymore if we're updating to net5.0. I took a wild guess and I didn't get lucky 🙂 I tried removing it entirely to see what would happen and I'm seeing: I don't suppose this is expected, given that the commit I linked modified the AddRefAssemblies target that's meant for the roslyn code task factory. |
|
|
There was a problem hiding this comment.
@benvillalobos please cherry-pick ladipro@4984166 to fix the failing StringTools tests.
src/Tasks/DownloadFile.cs
Outdated
|
|
||
| private bool ShouldSkip(HttpResponseMessage response, FileInfo destinationFile) | ||
| private bool ShouldSkip(HttpResponseMessage response, FileInfo destinationFile) |
There was a problem hiding this comment.
nit: Looks like an unintended whitespace change.
|
@KirillOsenkov you were the last to modify Also see #6148 (comment), I'm not entirely sure what to do with AddRefAssemblies. My impression at the moment is to construct a property that properly defines a path relative to each flavor OS. |
|
@benvillalobos You should be able to write a target that filters out those assemblies from what's resolved from the targeting packs instead of hardcoding the path. However, the .NET Core / .NET 5 versions of those assemblies may be different than reference assemblies of those for .NET Standard, so I'm not sure if they'll work anyway. |
|
@dsplaisted can you point me in the right direction for any properties set via these targeting packs that would help me accomplish this? |
|
@benvillalobos I'm saying you would filter out the items that come out of ResolveTargetingPackAssets based on filename. So you'd look for netstandard.dll and mscorlib.dll. However, I'm also not sure if the versions of those in the .NET 5 targeting pack will work for this or not. |
|
Did a test by hardcoding the dll's from the targeting packs: It doesn't error on copying the assembly, but this RoslynCodeTaskFactory test still fails |
|
Also seeing these failures: Some of these test failures have to do with the net472 mscorlib/netstandard dll's |
|
Any recent progress on this? Would be nice to finish it or close it until you're ready to come back to it. |
benvillalobos
force-pushed
the
netstandard2.0-to-net5.0
branch
2 times, most recently
from
89f2221
to
8ffe79f
Compare
| @@ -25,7 +25,7 @@ | |||
| <PackageReference Include="System.Configuration.ConfigurationManager" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETStandard'"> | |||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'"> | |||
src/Tasks/SignFile.cs
Outdated
| @@ -36,6 +39,9 @@ public SignFile() | |||
|
|
|||
| public string TimestampUrl { get; set; } | |||
|
|
|||
| #if RUNTIME_TYPE_NETCORE | |||
| [SupportedOSPlatform("windows")] | |||
There was a problem hiding this comment.
I don't think this should propagate quite so far. If we're going to expose the task we should expose it everywhere and emit good error messages if we hit a non-windows-compat codepath.
There was a problem hiding this comment.
I removed it from the Execute and appreciate this new way better than how it was. I'm not sure if we should go in and log errors in other areas where I applied SupportedOSPlatform? I think it makes sense to keep the static SignFile methods with this attribute, at least.
| @@ -37,7 +37,7 @@ public string GetOrCreateEntry(ref InternableString internable, out bool cacheHi | |||
|
|
|||
| // Get the existing handle from the cache and lock it while we're dereferencing it to prevent a race with the Scavenge | |||
| // method running on another thread and freeing the handle from underneath us. | |||
| if (_stringsByHashCode.TryGetValue(hashCode, out handle)) | |||
| if (_stringsByHashCode.TryGetValue(hashCode, out handle!)) | |||
There was a problem hiding this comment.
I think a better fix for this would be changing the definition of handle above to StringWeakHandle?
|
Investigation notes on Something is up with the <Project>
<UsingTask
TaskName="UseCores"
TaskFactory="RoslynCodeTaskFactory"
AssemblyFile="C:\src\git\msbuild\artifacts\bin\Microsoft.Build.Tasks.UnitTests\Debug\net6.0\Microsoft.Build.Tasks.Core.dll" >
<Task>
<Reference Include="C:\Program Files\dotnet\shared\Microsoft.NETCore.App\6.0.0-preview.3.21201.4\System.Runtime.dll" />
<Reference Include="C:\WINDOWS\Microsoft.Net\assembly\GAC_MSIL\System.Core\v4.0_4.0.0.0__b77a5c561934e089\System.Core.dll" />
<Code Type="Fragment" Language="cs">
{
System.Console.WriteLine("DEBUG");
}
</Code>
</Task>
</UsingTask>
<Target Name="Build">
<UseCores />
</Target>
</Project>
//------------------------------------------------------------------------------
// <auto-generated>
// This code was generated by a tool.
// Runtime Version:6.0.0
//
// Changes to this file may cause incorrect behavior and will be lost if
// the code is regenerated.
// </auto-generated>
//------------------------------------------------------------------------------
namespace InlineCode {
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
public class UseCores : Microsoft.Build.Utilities.Task {
private bool _Success = true;
public virtual bool Success {
get {
return _Success;
}
set {
_Success = value;
}
}
public override bool Execute() {
{
System.Console.WriteLine("DEBUG");
}
return Success;
}
}
}On deeper investigation, it passes the following references when trying to build: In a successful run (in VS unit tests), it passes the following: Messing around with the references passed into compilation, it always fails when passing the |
|
Seeing it written out like that made me go 💡. You're saying that Possibilities:
Thoughts? |
Seems like a reasonable thing to try. All assemblies seems opposed to the whole idea behind the PR though
Does |
Yeah, I'm worried we didn't fully understand the implications.
Correct. That gets us the assembly to put in our output package even though it's not referenced for us automatically (because we now autoreference .NET 6 stuff).
Yeah, it's pretty different. Leaving the reference as |
|
Latest internal build 🤞🤞🤞🤞🤞🤞 https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=5745444&view=results This should fix the issue upstream where ns2.0 ref assemblies were getting picked up by default because ref/net472 or ref/net6.0 didn't exist. Old Microsoft.Build.Tasks.Core NuGet package size Old Microsoft.Build.Tasks.Core NuGet package size |
src/Directory.Build.props
Outdated
| @@ -90,9 +94,13 @@ | |||
| </PropertyGroup> | |||
|
|
|||
| <!-- Ensure ns2.0 ref assemblies are placed under `ref/netstandard2.0` in the NuGet package --> | |||
src/Directory.Build.props
Outdated
| <ItemGroup> | ||
| <TfmSpecificPackageFile Include="@(BuiltProjectOutputGroupOutput);@(FinalDocFile)"> | ||
| <TfmSpecificPackageFile Include="@(IntermediateRefAssembly);@(FinalDocFile)"> |
There was a problem hiding this comment.
So I'd say you should use $(TargetRefPath) here; it should be identical for all realistic official build cases but that's the "final" ref assembly location; @(IntermediateRefAssembly) is written to by the compiler every time.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Also works if I reference the generated nuget package locally:
|
| @@ -10,7 +10,7 @@ | |||
| <ItemGroup> | |||
| <PackageReference Update="Microsoft.Build.NuGetSdkResolver" Version="$(NuGetBuildTasksVersion)" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Build.Tasks" Version="$(MicrosoftNetCompilersToolsetVersion)" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Collections" Version="4.0.0-4.21379.20" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Collections" Version="4.2.0-1.22102.8" /> | |||
There was a problem hiding this comment.
Do you have any exp/ insertions where you ran RPS? It'd also be nice if you could see why the optional test failed or find a known bug; lots of MSBuild-caused issues don't directly seem to mention MSBuild.
This change in particular sounds like it could cause version mismatches.
There was a problem hiding this comment.
I could've sworn I ran RPS on https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/380160. I'll retrigger on that PR.
There was a problem hiding this comment.
The full suite of tests should be queued: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/380160
| <LibraryTargetFrameworks Condition="'$(MonoBuild)'=='true'">$(FullFrameworkTFM)</LibraryTargetFrameworks> | ||
| <LibraryTargetFrameworks>$(FullFrameworkTFM);net6.0;netstandard2.0</LibraryTargetFrameworks> | ||
| <LibraryTargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">net6.0;netstandard2.0</LibraryTargetFrameworks> | ||
| <LibraryTargetFrameworks Condition="'$(MonoBuild)'=='true'">$(FullFrameworkTFM);netstandard2.0</LibraryTargetFrameworks> |
There was a problem hiding this comment.
About the monobuild case adding ns2.0? I added it to be safe because of how we're shipping ns2.0 ref assemblies now.
src/Directory.Build.props
Outdated
| <Target Name="ShipRefAssembliesToNuGetPackage" BeforeTargets="Pack" Condition="$(IsInnerBuild) == true"> | ||
| <ItemGroup> | ||
| <TfmSpecificPackageFile Include="$(TargetRefPath);@(FinalDocFile)"> | ||
| <PackagePath>ref/$(TargetFramework)</PackagePath> |
There was a problem hiding this comment.
nit:
| <PackagePath>ref/$(TargetFramework)</PackagePath> | |
| <PackagePath>ref\$(TargetFramework)</PackagePath> |
src/Directory.Build.props
Outdated
| </TfmSpecificPackageFile> | ||
| <!-- ns2.0 builds use `BuiltProjectOutputGroupOutput` for output ref assemblies --> | ||
| <TfmSpecificPackageFile Include="@(BuiltProjectOutputGroupOutput)" Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
| <PackagePath>ref/$(TargetFramework)</PackagePath> |
There was a problem hiding this comment.
| <PackagePath>ref/$(TargetFramework)</PackagePath> | |
| <PackagePath>ref\$(TargetFramework)</PackagePath> |
| </PropertyGroup> | ||
|
|
||
| <!-- Ensure ref assemblies are placed under `ref/$(TargetFramework)` in the NuGet package --> | ||
| <Target Name="ShipRefAssembliesToNuGetPackage" BeforeTargets="Pack" Condition="$(IsInnerBuild) == true"> |
There was a problem hiding this comment.
How is TfmSpecificPackageFile consumed?
There was a problem hiding this comment.
It's a nuget concept for packaging: https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#targetsfortfmspecificcontentinpackage
| if (actualException is IOException) | ||
| #if RUNTIME_TYPE_NETCORE | ||
| // net5.0 included StatusCode in the HttpRequestException. | ||
| switch (httpRequestException.StatusCode) |
There was a problem hiding this comment.
You should be able to put the status code in a variable and have less duplicated code.
There was a problem hiding this comment.
I don't feel strongly enough about this one
| @@ -386,7 +386,7 @@ private bool BuildResolvedSettings(ApplicationManifest manifest) | |||
| } | |||
| else if (String.IsNullOrEmpty(manifest.Publisher)) | |||
| { | |||
| string org = Util.GetRegisteredOrganization(); | |||
| string org = NativeMethodsShared.IsWindows ? Util.GetRegisteredOrganization() : string.Empty; | |||
There was a problem hiding this comment.
Bump rainersigwald's suggestion
src/Tasks/GenerateLauncher.cs
Outdated
| @@ -39,7 +39,7 @@ public sealed class GenerateLauncher : TaskExtension | |||
|
|
|||
| public override bool Execute() | |||
| { | |||
| if (LauncherPath == null) | |||
| if (LauncherPath == null && NativeMethodsShared.IsWindows) | |||
There was a problem hiding this comment.
Is this meant as a "don't run on windows"? Attribute to disable it would be much better here or making it more effectively crash.
There was a problem hiding this comment.
Somehow I skipped over this one. It's part of clickonce, so I'll mark the class as windows only and log "NotSupported" if Execute is called and we're not on windows.
| @@ -143,15 +139,15 @@ private static PermissionSet GetNamedPermissionSetFromZone(string targetZone, st | |||
|
|
|||
| private static PermissionSet GetNamedPermissionSet(string targetZone, string targetFrameworkMoniker) | |||
| { | |||
| FrameworkNameVersioning fn; | |||
| FrameworkName fn; | |||
There was a problem hiding this comment.
nit: ternary initializer --> saves 7 lines here, for instance.
| @@ -495,6 +491,7 @@ public static PermissionSet XmlToPermissionSet(XmlElement element) | |||
| /// <param name="certThumbprint">Hexadecimal string that contains the SHA-1 hash of the certificate.</param> | |||
| /// <param name="timestampUrl">URL that specifies an address of a time stamping server.</param> | |||
| /// <param name="path">Path of the file to sign with the certificate.</param> | |||
| [SupportedOSPlatform("windows")] | |||
There was a problem hiding this comment.
Should this just be applied to the class?
There was a problem hiding this comment.
I don't think so in this case. There are lots of internal static methods here that are valid cross-OS.
| @@ -15,7 +15,7 @@ | |||
| an imported package. This suppression should be removed if/when the project is migrated to enable nullable | |||
| reference types. | |||
| --> | |||
| <NoWarn>$(NoWarn),CS8632</NoWarn> | |||
| <NoWarn>$(NoWarn);CS8632</NoWarn> | |||
There was a problem hiding this comment.
Sorry to merge conflict but since you pointed this out: #7426.
There was a problem hiding this comment.
I undid this change so prevent a merge conflict.
…teApplicationManifest. Minor PR feedback
There was a problem hiding this comment.
Responding to final comments. Kicked DDRITs on my draft VS PR here: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/380160
| @@ -10,7 +10,7 @@ | |||
| <ItemGroup> | |||
| <PackageReference Update="Microsoft.Build.NuGetSdkResolver" Version="$(NuGetBuildTasksVersion)" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Build.Tasks" Version="$(MicrosoftNetCompilersToolsetVersion)" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Collections" Version="4.0.0-4.21379.20" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Collections" Version="4.2.0-1.22102.8" /> | |||
There was a problem hiding this comment.
I could've sworn I ran RPS on https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/380160. I'll retrigger on that PR.
| <LibraryTargetFrameworks Condition="'$(MonoBuild)'=='true'">$(FullFrameworkTFM)</LibraryTargetFrameworks> | ||
| <LibraryTargetFrameworks>$(FullFrameworkTFM);net6.0;netstandard2.0</LibraryTargetFrameworks> | ||
| <LibraryTargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">net6.0;netstandard2.0</LibraryTargetFrameworks> | ||
| <LibraryTargetFrameworks Condition="'$(MonoBuild)'=='true'">$(FullFrameworkTFM);netstandard2.0</LibraryTargetFrameworks> |
There was a problem hiding this comment.
About the monobuild case adding ns2.0? I added it to be safe because of how we're shipping ns2.0 ref assemblies now.
| </PropertyGroup> | ||
|
|
||
| <!-- Ensure ref assemblies are placed under `ref/$(TargetFramework)` in the NuGet package --> | ||
| <Target Name="ShipRefAssembliesToNuGetPackage" BeforeTargets="Pack" Condition="$(IsInnerBuild) == true"> |
There was a problem hiding this comment.
It's a nuget concept for packaging: https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#targetsfortfmspecificcontentinpackage
| /// <summary> | ||
| /// Gets a flag indicating if we are running under some version of Windows | ||
| /// </summary> | ||
| [SupportedOSPlatformGuard("windows")] |
There was a problem hiding this comment.
No it's still allowed, it prevents the compiler from complaining about windows only code paths. Basically, IsWindows itself is treated as a [SupportedOSPlatform("windows")]
| @@ -36,7 +36,7 @@ public SdkResultItem(string itemSpec, Dictionary<string, string>? metadata) | |||
| Metadata = metadata; | |||
| } | |||
|
|
|||
| public override bool Equals(object obj) | |||
| public override bool Equals(object? obj) | |||
There was a problem hiding this comment.
Created an issue for this: #7427
| if (actualException is IOException) | ||
| #if RUNTIME_TYPE_NETCORE | ||
| // net5.0 included StatusCode in the HttpRequestException. | ||
| switch (httpRequestException.StatusCode) |
There was a problem hiding this comment.
I don't feel strongly enough about this one
| @@ -386,7 +386,7 @@ private bool BuildResolvedSettings(ApplicationManifest manifest) | |||
| } | |||
| else if (String.IsNullOrEmpty(manifest.Publisher)) | |||
| { | |||
| string org = Util.GetRegisteredOrganization(); | |||
| string org = NativeMethodsShared.IsWindows ? Util.GetRegisteredOrganization() : string.Empty; | |||
There was a problem hiding this comment.
Blarg, I had a pending comment here I didn't publish. I'm sold on the idea of applying it to the whole class and erroring in the Execute method.
| using System.Runtime.Versioning; | ||
| #if RUNTIME_TYPE_NETCORE | ||
| using System.Runtime.InteropServices.ComTypes; | ||
| #endif |
There was a problem hiding this comment.
My brain says yes, my heart says no 😄
| @@ -495,6 +491,7 @@ public static PermissionSet XmlToPermissionSet(XmlElement element) | |||
| /// <param name="certThumbprint">Hexadecimal string that contains the SHA-1 hash of the certificate.</param> | |||
| /// <param name="timestampUrl">URL that specifies an address of a time stamping server.</param> | |||
| /// <param name="path">Path of the file to sign with the certificate.</param> | |||
| [SupportedOSPlatform("windows")] | |||
There was a problem hiding this comment.
I don't think so in this case. There are lots of internal static methods here that are valid cross-OS.
| @@ -10,7 +10,7 @@ | |||
| <ItemGroup> | |||
| <PackageReference Update="Microsoft.Build.NuGetSdkResolver" Version="$(NuGetBuildTasksVersion)" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Build.Tasks" Version="$(MicrosoftNetCompilersToolsetVersion)" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Collections" Version="4.0.0-4.21379.20" /> | |||
| <PackageReference Update="Microsoft.CodeAnalysis.Collections" Version="4.2.0-1.22102.8" /> | |||
There was a problem hiding this comment.
The full suite of tests should be queued: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/380160
There was a problem hiding this comment.
Pending successful exp/ insertion (green build/tests/RPS)
https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/380160
benvillalobos
added
the
merge-when-branch-open
benvillalobos
changed the title
This reverts commit 9c17329.
| @@ -273,7 +274,7 @@ private void init() | |||
| Sha256SignatureMethodUri); | |||
|
|
|||
| #if RUNTIME_TYPE_NETCORE | |||
| CryptoConfig.AddAlgorithm(typeof(SHA256Managed), | |||
| CryptoConfig.AddAlgorithm(typeof(SHA256), | |||
There was a problem hiding this comment.
@benvillalobos do you remember why it was needed to switch from SHA256Managed?
We have a bug now, I am thinking about getting back to SHA256Managed
Fixes #6032
Context
While we're in the process of updating target frameworks, let's move off of netstandard2.0.
EXCEPT: We need two projects (M.B.Framework and M.B.Utilities) to build as ns2.0 and place their ref assemblies under
OutputPath/reffor the RoslynCodeTaskFactory.Changes Made
Changed
TargetLibraryFrameworksto includenet6.0Add a target for ns2.0 builds to place M.B.Framework and Utilities ref assemblies into the build's
ref/folder.Marked various tasks (Particularly SignFile) with a "Windows" OS Attribute
Testing
CI and local builds pass.
Notes
Review by diff!