-
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 5.0 and .NET 5.0 #5836
Conversation
539c1d4
to
f716da7
Compare
Note to self: Update |
Running the CI script locally results in: 3.1.100
5.0.100-rc.2.20479.15
Same issues for |
This is very curious...
Note to self: My local directory has Or did me migrating to |
I can't tell if this is "so close, yet so far," but only full framework is failing on The error is this:
Previous error:
Updating |
Can you temporarily change only the .NET Core version of S.T.Json? And keep the net472 version as is?
|
@rainersigwald strange, I added this in packages.props (keeping the original package reference that sets s.t.json to 4.7.0 some lines above):
and I get the same error
|
Only importing |
And on this glorious night the old gods and the new smiled down on Ben for the first time and said "Tests passed!" |
|
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Looks like pipeline builds succeed. We get to the VS PR stage and RPS passes. The only failure at the VS PR stage is symbolcheck:
|
The symchk bugs should have a repro command line. Can you run that? |
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.
Looking super promising!
eng/Versions.props
Outdated
@@ -36,8 +36,8 @@ | |||
</PropertyGroup> | |||
<!-- Toolset Dependencies --> | |||
<PropertyGroup> | |||
<DotNetCliVersion>3.1.100</DotNetCliVersion> | |||
<MicrosoftNetCompilersToolsetVersion>3.3.1-beta3-final</MicrosoftNetCompilersToolsetVersion> | |||
<DotNetCliVersion>5.0.100</DotNetCliVersion> |
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.
Should probably be .101 (or .102) now.
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.
Where can I go to verify this? I did a brief search in the sdk and arcade repos
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.
https://dotnet.microsoft.com/download/dotnet/5.0 says 102 is current latest
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.
🤦♂️ 👍
src/Directory.BeforeCommon.targets
Outdated
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="$(TargetFramework.StartsWith('netstandard')) or $(TargetFramework.StartsWith('netcore'))"> | ||
<PropertyGroup Condition="$(TargetFramework.StartsWith('netstandard')) or $(TargetFramework.StartsWith('netcore')) or $(TargetFramework.StartsWith('net5'))"> |
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.
Replace with is-compatible-with-netcoreapp1.0 property function?
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.
Are all netcoreapp1.0+ target frameworks backwards compatible that far?
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.
theoretically yes
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.
Well this is strange. Changing this to: $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netcoreapp1.0'))
results in a compilation error:
C:\src\git\msbuild\src\Tasks\AssemblyDependency\AssemblyInformation.cs(34,26): error CS0246: The type or namespace name
'IMetaDataDispenser' could not be found (are you missing a using directive or an assembly reference?) [C:\src\git\msbu
ild\src\Tasks\Microsoft.Build.Tasks.csproj]
C:\src\git\msbuild\src\Tasks\AssemblyDependency\AssemblyInformation.cs(35,26): error CS0246: The type or namespace name
'IMetaDataAssemblyImport' could not be found (are you missing a using directive or an assembly reference?)
and others
The failing project is Microsoft.Build.Tasks.csproj targeting netstandard2.0. This condition must have been false for netstandard2.0 previously?
Here's a sample of the failing code:
#if !FEATURE_ASSEMBLYLOADCONTEXT
if (NativeMethodsShared.IsWindows)
{
// Create the metadata dispenser and open scope on the source file.
_metadataDispenser = (IMetaDataDispenser)new CorMetaDataDispenser();
_assemblyImport = (IMetaDataAssemblyImport)_metadataDispenser.OpenScope(sourceFile, 0, ref s_importerGuid);
}
else
{
_assembly = Assembly.ReflectionOnlyLoadFrom(sourceFile);
}
#endif
My first assumption would be that the netstandard2.0 project didn't have FEATURE_ASSEMBLYLOADCONTEXT defined, and now it is and doesn't see those types.
- This is even more confusing considering the previous check was checking if the TargetFramework started with
netstandard
, so it would've fallen into this case anyway?
So it seems like there are two paths here:
- Get the type to show up in the netstandard2.0 target (which doesn't seem right to me considering it apparently wasn't visible before),
- Compare the two binlogs and see what's different between the current condition and the previous.
-
- Ex: See if the previous condition had FEATURE_ASSEMBLYLOADCONTEXT defined.
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.
I think I see the problem. netcoreapp1.0
is not compatible with netstandard2.0
, so I think I need to change the condition to "is target framework compatible with netcoreapp1.0 OR does the target framework start with netstandard"
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.
I figured that by seeing what constants were defined in a build with the old condition vs the new condition.
Working (Old condition)
----
TRACE
MICROSOFT_BUILD_TASKS
STANDALONEBUILD
FEATURE_DEBUG_LAUNCH
TEST_ISWINDOWS
RUNTIME_TYPE_NETCORE
FEATURE_ASSEMBLYLOADCONTEXT
FEATURE_PROCESSSTARTINFO_ENVIRONMENT
FEATURE_RUNTIMEINFORMATION
USE_MSBUILD_DLL_EXTN
WORKAROUND_COREFX_19110
DEBUG
NETSTANDARD
NETSTANDARD2_0
Not Working (new condition)
----
TRACE
MICROSOFT_BUILD_TASKS
STANDALONEBUILD
FEATURE_DEBUG_LAUNCH
TEST_ISWINDOWS
DEBUG
NETSTANDARD
NETSTANDARD2_0
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.
And that fixed it 👍
@@ -45,10 +45,10 @@ | |||
</ItemGroup> | |||
|
|||
<!-- Use deps file from this project with additional dependencies listed instead of the one generated in the MSBuild project --> | |||
<Target Name="UpdateMSBuildDepsFile" AfterTargets="Build" Condition="'$(TargetFramework)' == 'netcoreapp2.1' or '$(TargetFramework)' == 'netstandard2.0'"> | |||
<Target Name="UpdateMSBuildDepsFile" AfterTargets="Build" Condition="'$(TargetFramework)' == 'net5.0' or '$(TargetFramework)' == 'netstandard2.0'"> |
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.
Compat property functions (and next target condition)
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.
I tried using "$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net5.0')) or $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netstandard2.0'))
and hit issues with net472:
C:\src\git\msbuild\src\MSBuild.Bootstrap\MSBuild.Bootstrap.csproj(49,5): error MSB3030: Could not copy the file "C:\src\g
it\msbuild\artifacts\bin\MSBuild.Bootstrap\Debug\net472\MSBuild.Bootstrap.deps.json" because it was not found.
Should I be using a different intrinsic function here?
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.
Ah, unfortunate. That's right but net472
is compatible with netstandard2.0
so it's a bit more complicated. Can it be just compatible with netcoreapp2.1
to catch our core flavors? We're not building netstandard anything anymore right/
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.
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.
I don't think there's a good reason to not retarget that. But it doesn't have to be in this PR. Reasons to do it: use new APIs etc in tasks, which is a good thing.
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.
I quickly updated my comment after noticing my query was looking for the one project. There are many that target netstandard2.0. Seems like this should be tracked and figured out in a different issue.
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.
Sounds like a plan but let's prioritize that soon after this so we don't forget.
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 to clarify, you want to update them to netstandard2.1
?
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.
Tracking here: #6032
The new condition definitely doesn't look any cleaner than before, but it's more future-proof.
This reverts commit bcfa960. Context from Rainer: Arcade has an implicit version for microsoft.net.compilers.toolset. It's ancient in arcade 1.0, newer in 5.0. We used to depend on the implicit version, so when I moved arcade it changed. Rainer recently made it explicit to the older version, thus we need to look back at netcoreapp2.1
…ain tests would find MSBuild.exe AND MSBuild.dll when they should have only found msbuild.dll
The with-a-name ctor wasn't available in .NET Core 2.1 but now is, so use it and restore the test.
…ces.Unsafe to 5.0.0
…, short-circuiting the nonexistent IsTargetFrameworkCompatible intrinsic function on mono
77daa88
to
d1a83ca
Compare
…date the version of DotNetCliVersion in versions.props AND global.json
…coreapp1.0 OR if TargetFramework starts with netstandard
… function (that doesn't exist in mono builds)
This merge is blocked pending #6031 |
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.
Approving on rainersigwald's behalf.
Co-authored-by: Rainer Sigwald <[email protected]> Now on dotnet 5.0.102 Removed OsEnvironment in favor of IsOSPlatform intrinsic function Nowarn on SYSLIB0011 Update various build scripts with net5.0 path Use IsTargetFrameworkCompatible in various locations to simplify checks.
Co-authored-by: Rainer Sigwald <[email protected]> Now on dotnet 5.0.102 Removed OsEnvironment in favor of IsOSPlatform intrinsic function Nowarn on SYSLIB0011 Update various build scripts with net5.0 path Use IsTargetFrameworkCompatible in various locations to simplify checks.
This reverts commit f98579d.
Fixes #5515
Ran
darc update-dependencies --source-repo arcade --channel ".NET 5 Eng"
as opposed to ".NET Eng - Latest" as latest was on 6.0.Validation:
/exp
BuildsExtras
Questions
Is there any reason we wouldn't want to jump straight to 6.0?