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

[master] Update dependencies from dotnet/runtime #45387

Merged
merged 27 commits into from
Dec 16, 2020

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Nov 30, 2020

This pull request updates the following dependencies

From https://github.com/dotnet/runtime

  • Subscription: a67af1d4-463b-4caf-856e-08d895558180
  • Build: 20201214.10
  • Date Produced: 12/15/2020 8:14 AM
  • Commit: c64861b
  • Branch: refs/heads/master

…1130.7

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20580.7
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

dotnet-maestro bot and others added 2 commits December 1, 2020 13:17
…1201.3

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20601.3
'JsonSerializerOptions.IgnoreNullValues' is obsolete: 'To ignore null values when serializing, set DefaultIgnoreCondition to JsonIgnoreCondition.WhenWritingNull.'
@akoeplinger
Copy link
Member

Seems we're running into Classic PDB format is not supported on CoreCLR. error with the new ilasm.

Looks like this is because of 99bbb19 which added support for writing PDBs when /DEBUG is passed to ilasm, but the default seems to be /PDBFMT=CLASSIC which doesn't work on coreclr.

@noahfalk @BruceForstall @AaronRobinsonMSFT I'm not sure if we can easily detect coreclr in src/coreclr/src/.nuget/Microsoft.NET.Sdk.IL/targets/Microsoft.NET.Sdk.IL.targets so we can pass /PDBFMT=PORTABLE, any ideas?

@BruceForstall
Copy link
Member

I don't know how those targets file work, esp. for Core vs. non-Core. Isn't the referenced Microsoft.NET.Sdk.IL.targets only for coreclr? In which case maybe anything passing -DEBUG should also pass /PDBFMT=PORTABLE?

@davidwrighton comments?

…1202.2

runtime.native.System.IO.Ports , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , Microsoft.NETCore.ILAsm , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20602.2
@akoeplinger
Copy link
Member

akoeplinger commented Dec 2, 2020

Actually, isn't the ilasm built from this repo always using coreclr (since the sources aren't shared back with netframework afaik), so maybe we should just default to portable PDB in ilasm?

@BruceForstall
Copy link
Member

You could presumably use the dotnet/runtime ilasm to build code to be used for .NET Framework, but that seems unlikely. More likely, there was probably a desire not to change existing command-line behavior. I'm not sure what the correct default should be.

@ViktorHofer
Copy link
Member

I think switching to portable as the default sounds reasonable but I'm not an expert in that area. cc @ericstj

@akoeplinger
Copy link
Member

akoeplinger commented Dec 3, 2020

The netframework version of ilasm doesn't have the /PDBFMT option so there shouldn't be any compatibility issues.

If I understand @AaronRobinsonMSFT comment on #39436 (comment) correctly it sounds like the intention was indeed to use Portable PDB as the default, it just didn't happen? actually now that I read it again it seems keeping Classic as the default was intentional, however it feels kinda wrong to me if it doesn't work anyway.

@ericstj
Copy link
Member

ericstj commented Dec 3, 2020

At least the ILSDK targets should be made to understand the same properties that CSC uses to set the PDB type. That'll resolve the warning/error. As far as defaults, that can be a different discussion.

@akoeplinger
Copy link
Member

akoeplinger commented Dec 3, 2020

@ericstj I'm not sure it'd fix the build error since we seem to have some cases where we actually pass DebugType=Full otherwise we wouldn't pass /DEBUG to ikdasm:

<_IlasmSwitches Condition="'$(DebugType)' == 'Full'">$(_IlasmSwitches) -DEBUG</_IlasmSwitches>
<_IlasmSwitches Condition="'$(DebugType)' == 'Impl'">$(_IlasmSwitches) -DEBUG=IMPL</_IlasmSwitches>
<_IlasmSwitches Condition="'$(DebugType)' == 'PdbOnly'">$(_IlasmSwitches) -DEBUG=OPT</_IlasmSwitches>

@ericstj
Copy link
Member

ericstj commented Dec 3, 2020

Fix that too then :) Here:

<IlasmFlags>$(IlasmFlags) -DEBUG=$(DebugOptimization)</IlasmFlags>

dotnet-maestro bot and others added 8 commits December 3, 2020 13:28
…1203.3

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20603.3
…1203.13

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20603.13
…1204.10

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20604.10
…1205.2

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20605.2
…1206.6

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20606.6
…1208.2

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NETCore.DotNetHostPolicy , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20608.2
@radical
Copy link
Member

radical commented Dec 8, 2020

Now ilasm failing with seg fault:

Target "CoreCompile: (TargetId:11731)" in file "/__w/1/s/.packages/microsoft.net.sdk.il/6.0.0-alpha.1.20608.2/targets/Microsoft.NET.Sdk.IL.targets" from project "/__w/1/s/src/libraries/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerSe
Building target "CoreCompile" completely.
Output file "/__w/1/s/artifacts/obj/System.Runtime.CompilerServices.Unsafe/netstandard2.0-Release/System.Runtime.CompilerServices.Unsafe.dll" does not exist.
Set Property: _OutputTypeArgument=-DLL
Set Property: _KeyFileArgument=-KEY="/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/6.0.0-beta.20601.2/tools/snk/MSFT.snk"
Set Property: _IlasmSwitches=-QUIET -NOLOGO
Set Property: _IlasmSwitches=-QUIET -NOLOGO -OPTIMIZE
Task "Exec" (TaskId:7532)
  Task Parameter:Command="/__w/1/s/.packages/runtime.linux-x64.microsoft.netcore.ilasm/6.0.0-alpha.1.20608.2/runtimes/linux-x64/native/ilasm" -QUIET -NOLOGO -OPTIMIZE -DLL  -DEBUG=OPT -PDBFMT=PORTABLE -INCLUDE="/__w/1/s/artifacts/obj/System.Runtime.Comp
  "/__w/1/s/.packages/runtime.linux-x64.microsoft.netcore.ilasm/6.0.0-alpha.1.20608.2/runtimes/linux-x64/native/ilasm" -QUIET -NOLOGO -OPTIMIZE -DLL  -DEBUG=OPT -PDBFMT=PORTABLE -INCLUDE="/__w/1/s/artifacts/obj/System.Runtime.CompilerServices.Unsafe/net
  Segmentation fault (core dumped) (TaskId:7532)
/__w/1/s/.packages/microsoft.net.sdk.il/6.0.0-alpha.1.20608.2/targets/Microsoft.NET.Sdk.IL.targets(138,5): error MSB3073: The command ""/__w/1/s/.packages/runtime.linux-x64.microsoft.netcore.ilasm/6.0.0-alpha.1.20608.2/runtimes/linux-x64/native/ilasm" -
  Output Property: _ILAsmExitCode=139 (TaskId:7532)
Done executing task "Exec" -- FAILED. (TaskId:7532)
Done building target "CoreCompile" in project "System.Runtime.CompilerServices.Unsafe.ilproj" -- FAILED.: (TargetId:11731)

https://dev.azure.com/dnceng/public/_build/results?buildId=912359&view=logs&jobId=108d2c4a-8a62-5a58-8dad-8e1042acc93c&j=108d2c4a-8a62-5a58-8dad-8e1042acc93c&t=568f884b-cc12-5fd3-e7fe-790b5ac403f4

…1209.2

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NETCore.DotNetHostPolicy , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20609.2
…1209.15

runtime.native.System.IO.Ports , Microsoft.NETCore.ILAsm , Microsoft.NETCore.DotNetHostPolicy , Microsoft.NET.Sdk.IL , Microsoft.NETCore.DotNetHost , System.Text.Json
 From Version 5.0.0-alpha.1.19563.3 -> To Version 6.0.0-alpha.1.20609.15
akoeplinger
akoeplinger previously approved these changes Dec 15, 2020
@jkotas
Copy link
Member

jkotas commented Dec 15, 2020

Ugh, that is a lot of files that need this workaround. I doubt that this set is complete. I think we should get the proper fix in ilasm in place instead.

@akoeplinger
Copy link
Member

akoeplinger commented Dec 15, 2020

@jkotas I did run every .il file in src/tests through ilasm manually and fixed all of the cases where it crashed so I'm reasonably sure it is the complete set.

@jkotas
Copy link
Member

jkotas commented Dec 15, 2020

Still, I think it would be better to get the proper fix in ilasm instead of touching 400+ files.

@jkotas
Copy link
Member

jkotas commented Dec 15, 2020

#46080 has the fix for the ilasm crash. Could you please revert the workaround and wait for it get merged?

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Just to prevent the PR from being auto-merged accidentally.

@ViktorHofer
Copy link
Member

Failure is #11063.

@ViktorHofer ViktorHofer merged commit 44e6223 into master Dec 16, 2020
@ViktorHofer ViktorHofer deleted the darc-master-8672eaad-b7a4-4bb1-8166-9393d926e7a6 branch December 16, 2020 13:19
@ghost ghost locked as resolved and limited conversation to collaborators Jan 15, 2021
@danmoseley danmoseley added the area-codeflow for labeling automated codeflow label Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-codeflow for labeling automated codeflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants