Install netfx dlls into GAC #1884

pellared · 2023-01-03T10:02:33Z

Why

What

Install and uninstall netfx dlls into GAC via OpenTelemetry.DotNet.Auto.psm1

Tests

Manual

Test an Fx application referencing an older version of the System.Memory dll or something used by OTLP should cover it. In this case, if the needed dll is not on the GAC and we load a newer version from the netfx folder the runtime will error because the application actually is shipping a different version. I think I added some workaround to these issues to some specific tests when I did the netfx redirection. Let's check that.

Introduce a test failure

$ git diff
diff --git a/test/test-applications/integrations/TestApplication.StrongNamed/Program.cs b/test/test-applications/integrations/TestApplication.StrongNamed/Program.cs
index 3c00b352d..ddf0e49f3 100644
--- a/test/test-applications/integrations/TestApplication.StrongNamed/Program.cs
+++ b/test/test-applications/integrations/TestApplication.StrongNamed/Program.cs
@@ -14,6 +14,7 @@
 // limitations under the License.
 // </copyright>
 
+using System;
 using TestLibrary.InstrumentationTarget;
 
 namespace TestApplication.StrongNamed;
@@ -22,6 +23,10 @@ public static class Program
 {
     public static void Main(string[] args)
     {
+        var arrayMemory = new byte[100];
+        var arraySpan = new Span<byte>(arrayMemory);
+        Console.WriteLine(arraySpan.ToString());
+
         var command = new Command();
         command.Execute();
         command.InstrumentationTargetMissingBytecodeInstrumentationType();
diff --git a/test/test-applications/integrations/TestApplication.StrongNamed/TestApplication.StrongNamed.csproj b/test/test-applications/integrations/TestApplication.StrongNamed/TestApplication.StrongNamed.csproj
index f21f7478c..3ce15ced7 100644
--- a/test/test-applications/integrations/TestApplication.StrongNamed/TestApplication.StrongNamed.csproj
+++ b/test/test-applications/integrations/TestApplication.StrongNamed/TestApplication.StrongNamed.csproj
@@ -8,4 +8,8 @@
     <ProjectReference Include="..\dependency-libs\TestLibrary.InstrumentationTarget\TestLibrary.InstrumentationTarget.csproj" />
   </ItemGroup>

+  <ItemGroup>
+    <PackageReference Include="System.Memory" Version="4.5.0" />
+  </ItemGroup>
+
 </Project>

Run nuke Workflow --test-name StrongNamedTests --test-project Integration
TEST FAILED

Execute in PowerShell (as admin):

Import-Module .\OpenTelemetry.DotNet.Auto.psm1
Install-OpenTelemetryCore

Run nuke RunManagedIntegrationTests --test-name StrongNamedTests
TEST PASSED 🎉
Run Uninstall-OpenTelemetryCore
TEST FAILED 🎉

Checklist

CHANGELOG.md is updated.
- Q: Is it worth mentioning?
Documentation is updated.
- Q: Is it worth mentioning?
New features are covered by tests.
- Q: Is it worth having some automated test?

pellared · 2023-01-03T11:31:44Z

@pjanotti Is there anything I am missing? Any recommendation on how to test it?

pellared · 2023-01-03T13:11:17Z

We need a friendly way to install the assemblies shipped under the netfx folder into the GAC. This is necessary in the case the application and auto instrumentation ship different versions of the same assembly.

Do we want to add strong names for all of the assemblies so that they can be added to GAC?

pjanotti · 2023-01-03T16:27:52Z

@pellared

Is there anything I am missing?

This should cover it.

Any recommendations on how to test it?

Test an Fx application referencing an older version of the System.Memory dll or something used by OTLP should cover it. In this case, if the needed dll is not on the GAC and we load a newer version from the netfx folder the runtime will error because the application actually is shipping a different version. I think I added some workaround to these issues to some specific tests when I did the netfx redirection. Let's check that.

pjanotti · 2023-01-03T16:40:54Z

Do we want to add strong names for all of the assemblies so that they can be added to GAC?

🤔 I'm unsure at this point. In principle, this is mostly for DLLs shipped by the actual Fx applications themselves but which ones would we need besides OpenTelemetry.AutoInstrumentation? That said it may simplify things on our side if we just strong name everything on the netfx folder.

pjanotti · 2023-01-03T16:41:22Z

OpenTelemetry.DotNet.Auto.psm1

+        $publish = New-Object System.EnterpriseServices.Internal.Publish 
+        $dlls = Get-ChildItem -Path $installDir\netfx\ -Filter *.dll -File
+        foreach ($dll in $dlls) {
+            $publish.GacInstall($dll)


Q.: what happens if the dll is not strong-named?

Right now only 3 DLLs (grpc_csharp_ext.x64.dll, grpc_csharp_ext.x86.dll, netstandard.dll) are not added to GAC and if it fails then we have a warning log in Event Viewer

The grpc_csharp_ext.x64.dll, grpc_csharp_ext.x86.dll are not .NET assemblies.

netstandard.dll was probably not added because it was already in GAC.

I think we do not have to do anything at this moment.

PS. Fix cf1c304

pjanotti · 2023-01-03T16:47:42Z

OpenTelemetry.DotNet.Auto.psm1

+    $publish = New-Object System.EnterpriseServices.Internal.Publish 
+    $dlls = Get-ChildItem -Path $installDir\netfx\ -Filter *.dll -File
+    foreach ($dll in $dlls) {
+        $publish.GacRemove($dll)


This can be a problem with the GAC because by itself it doesn't keep some kind of reference counting, so we may end up removing something added by other applications. I think MSIs interact with some low-level Windows APIs to do some kind of reference counting.

Short of using an MSI we may want to look into whatever API MSIs use for this purpose. That said we should treat as a separate issue and address it in a separate issue/PR.

There is a chance that $publish.GacRemove($dll) has some logic to mitigate the problem as netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL has not been removed by this line 😉

For the record: assemblies added to the GAC by MSIs aren't removed by $publish.GacRemove. Assemblies added to the GAC via $publish.GacInstall are not protected in the same way and multiple additions from different locations for the same assembly are clobbered by a single $publish.GacRemove.

Some more info related to the GAC usage at #1906 (comment)

pellared · 2023-01-03T18:45:24Z

Tested manually (described in PR description). Ready for review.

Install netfx dlls into GAC

2f4f6ba

github-actions bot requested a review from theletterf January 3, 2023 10:02

Merge branch 'main' into gac-netfx

f62e60e

pellared requested a review from pjanotti January 3, 2023 11:32

pjanotti reviewed Jan 3, 2023

View reviewed changes

pellared added 2 commits January 3, 2023 18:18

Better output (silent)

06712bc

Fix the bad path

cf1c304

pellared marked this pull request as ready for review January 3, 2023 18:45

pellared requested a review from a team January 3, 2023 18:45

pjanotti approved these changes Jan 3, 2023

View reviewed changes

RassK approved these changes Jan 3, 2023

View reviewed changes

Merge branch 'main' into gac-netfx

0cd783b

pjanotti merged commit 1aa9403 into open-telemetry:main Jan 3, 2023

pjanotti mentioned this pull request Jan 9, 2023

GAC - install and uninstall clarification #1906

Closed

pellared deleted the gac-netfx branch January 17, 2023 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install netfx dlls into GAC #1884

Install netfx dlls into GAC #1884

pellared commented Jan 3, 2023 •

edited

Loading

pellared commented Jan 3, 2023

pellared commented Jan 3, 2023

pjanotti commented Jan 3, 2023

pjanotti commented Jan 3, 2023

pjanotti Jan 3, 2023

pellared Jan 3, 2023 •

edited

Loading

pjanotti Jan 3, 2023

pellared Jan 3, 2023 •

edited

Loading

pjanotti Jan 3, 2023

pjanotti Jan 12, 2023

pjanotti Jan 12, 2023

pellared commented Jan 3, 2023

Install netfx dlls into GAC #1884

Install netfx dlls into GAC #1884

Conversation

pellared commented Jan 3, 2023 • edited Loading

Why

What

Tests

Checklist

pellared commented Jan 3, 2023

pellared commented Jan 3, 2023

pjanotti commented Jan 3, 2023

pjanotti commented Jan 3, 2023

pjanotti Jan 3, 2023

Choose a reason for hiding this comment

pellared Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

pjanotti Jan 3, 2023

Choose a reason for hiding this comment

pellared Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

pjanotti Jan 3, 2023

Choose a reason for hiding this comment

pjanotti Jan 12, 2023

Choose a reason for hiding this comment

pjanotti Jan 12, 2023

Choose a reason for hiding this comment

pellared commented Jan 3, 2023

pellared commented Jan 3, 2023 •

edited

Loading

pellared Jan 3, 2023 •

edited

Loading

pellared Jan 3, 2023 •

edited

Loading