Maoni0 / realmon

A monitoring tool that tells you when GCs happen in a process and some characteristics about these GCs
MIT License
281 stars 25 forks source link

Remove workaround for NativeAOT #15

Closed hez2010 closed 2 years ago

hez2010 commented 2 years ago

Since https://github.com/dotnet/runtimelab/pull/1728 fixed the issue where some metadata is missing after compilation, I removed the workaround in rd.xml.

Also fixes #20

hez2010 commented 2 years ago

This PR should be held until the new ILCompiler build being uploaded to package source. Will remove the DO NOT MERGE in title once it's ready.

hez2010 commented 2 years ago

This PR is now ready for merge.

Maoni0 commented 2 years ago

hi @hez2010, so the resulting binary doesn't seem to work with the latest src, with or without this change... this is what I got

C:\realmon>C:\realmon\src\GCRealTimeMon\bin\Release\net6.0\win-x64\publish\gcrealtimemon -p 16128
Value cannot be null. Parameter name: path1

can you please try and see if this is what you get? I also got this build error -

C:\Program Files\dotnet\sdk\6.0.100-rc.2.21505.57\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.PackTool.targets(86,5): error NETSDK1053: Pack as tool does not support self contained. [C:\realmon\src\dotnet-gcmon\dotnet-gcmon.csproj]

looks like we shouldn't be trying to AoT this one or C:\realmon\tests\GCRealTimeMon.UnitTests\bin\Release\net6.0\win-x64\GCRealTimeMon.UnitTests.dll.

hez2010 commented 2 years ago

Will investigate. Seems that some commits break the NativeAOT compilation so that I need to add more runtime directives.

hez2010 commented 2 years ago

@Maoni0 Done. I've also added support for .NET 5.0 and .NET Core 3.1.

Maoni0 commented 2 years ago

BTW, it would also be good to have some automatic testing for this because the AoTed version probably gets used very little. I don't use it myself and it looks like most people use the tool via the dotnet tools route.

hez2010 commented 2 years ago

I think we need to keep net6.0 target to satisfy CI. Both netcoreapp3.1 and net5.0 are about to reach EOL. And seems that one can't run netcoreapp3.1 build of dotnet-gcmon if there's no .NET Core 3.1 runtime installed on the machine despite .NET 6 is present.

chrisnas commented 2 years ago

I think we need to keep net6.0 target to satisfy CI. Both netcoreapp3.1 and net5.0 are about to reach EOL. And seems that one can't run netcoreapp3.1 build of dotnet-gcmon if there's no .NET Core 3.1 runtime installed on the machine despite .NET 6 is present.

I don't know how dotnet tool install -g knows which "version" to extract from the nuget package... The EOL is a Microsoft concept that is not "always" taken into account on customer sites (believe me ;^) and 3.1 should be kept for much longer. The only possible issue (i.e. having 3.1 only) that crossed my mind was forward compatibility; i.e. 3.1 gcmon trying to monitor a .NET 6 application. I tested it and it works (basically because the tool is run by the CLR 6)

hez2010 commented 2 years ago

I don't know how dotnet tool install -g knows which "version" to extract from the nuget package...

dotnet tool install doesn't know the exact runtime version from nuget. There's a .runtimeconfig.json present in the tool directory to indicate the proper runtime needed by the tool, and dotnet cli will choose a proper one to execute.

So most tools are multi-targeted and they organize their content as following:

| net6.0
  \ - tool.dll
| net5.0
  \ - tool.dll
| netcoreapp3.1
  \ - tool.dll
.....

If there's no proper version of dotnet runtime installed on the machine, trying to run the tool will fail.

chrisnas commented 2 years ago

I don't know how dotnet tool install -g knows which "version" to extract from the nuget package...

dotnet tool install doesn't know the exact runtime version from nuget. There's a .runtimeconfig.json present in the tool directory to indicate the proper runtime needed by the tool, and dotnet cli will choose a proper one to execute.

So most tools are multi-targeted and they organize their content as following:

| net6.0
  \ - tool.dll
| net5.0
  \ - tool.dll
| netcoreapp3.1
  \ - tool.dll
.....

If there's no proper version of dotnet runtime installed on the machine, trying to run the tool will fail.

The .NET Diagnostics teams decided to keep only netcoreapp3.1 for all other CLI global tools. image

I think we should do the same

hez2010 commented 2 years ago

I think we should do the same

I've downgraded the dotnet version in CI to 3.1.

@Maoni0 PTAL

Maoni0 commented 2 years ago

aside from that one minor comment (and conflicts need to be resolved), the rest LGTM!

hez2010 commented 2 years ago

@Maoni0 Merge conflict has been resolved

Maoni0 commented 2 years ago

thanks @hez2010!!