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

Install netfx dlls to GAC for tests #2164

Merged

Conversation

RassK
Copy link
Contributor

@RassK RassK commented Feb 3, 2023

Why

Fixes #2155

What

Installs tracer-home/netfx dlls to GAC before running integration tests.

Tests

Existing.

Checklist

  • New features are covered by tests.

@RassK RassK requested a review from a team February 3, 2023 15:01
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

@RassK What do you think about providing an uninstall target? It seems useful during development. While we don't need it in CI it helps keep the dev box clean.

tools/GacInstallTool/Program.cs Outdated Show resolved Hide resolved
tools/GacInstallTool/Program.cs Outdated Show resolved Hide resolved
@RassK
Copy link
Contributor Author

RassK commented Feb 3, 2023

@RassK What do you think about providing an uninstall target? It seems useful during development. While we don't need it in CI it helps keep the dev box clean.

I really wanted to implement it but I think it's not that straightforward. I'm a bit afraid to remove something that overlaps and breaks other applications. To fix that I would need to save a state that tracks what's new and could be removed... although even this is valid only short time.

build/Build.Steps.Windows.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

I think that we should update documentation that instrumenting .NET Fx stuff on Windows requires GAC update (done by psm1 file).
I do not think that it is obvious https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/main/docs/README.md?plain=1#L79-L211

tools/GacInstallTool/GacInstallTool.csproj Show resolved Hide resolved
@@ -177,4 +179,20 @@ partial class Build

AssemblyRedirectionSourceGenerator.Generate(netFxAssembliesFolder, generatedSourceFile);
});

Target InstallNetFxAssembliesGAC => _ => _
Copy link
Contributor

Choose a reason for hiding this comment

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

If I execute nuke or nuke Workflow with elevated privileges it will overwrite GAC,
It will be great to have reverse step included into it, or at least provide easy way to remove all this files.

I think nuke Workflow CleanupGAC execution form is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have it as part of nuke or nuke Workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 2 ways which might work according to your own risk.

  1. Add CleanupGAC to the workflow and a warning to use it on your own risk or skip the step.
  2. Add CleanupGAC as a non referenced step. You need to run it yourself manually.

Copy link
Member

Choose a reason for hiding this comment

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

Probably point 2 is better/safer after all you said 😉
Make it could somehow part of nuke Cleanup?

Copy link
Contributor Author

@RassK RassK Feb 6, 2023

Choose a reason for hiding this comment

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

Added UninstallNetFxAssembliesGAC also.
Clean is unfortunately way too early, tracer-home may not be present or in an outdated state.

@pellared
Copy link
Member

pellared commented Feb 6, 2023

I really wanted to implement it but I think it's not that straightforward. I'm a bit afraid to remove something that overlaps and breaks other applications. To fix that I would need to save a state that tracks what's new and could be removed... although even this is valid only short time.

@pjanotti was testing it and it will not remove the assemblies from GAC that were added by MSIs. See #1906 (comment)

@RassK
Copy link
Contributor Author

RassK commented Feb 6, 2023

I really wanted to implement it but I think it's not that straightforward. I'm a bit afraid to remove something that overlaps and breaks other applications. To fix that I would need to save a state that tracks what's new and could be removed... although even this is valid only short time.

@pjanotti was testing it and it will not remove the assemblies from GAC that were added by MSIs. See #1906 (comment)

Unfortunately there is no knowledge how different libs were added. It might work for CI or isolated work environment but general development machines (used for multiple projects) could be affected by removal.

build/Build.Steps.Windows.cs Outdated Show resolved Hide resolved
build/Build.Steps.Windows.cs Outdated Show resolved Hide resolved
@pjanotti pjanotti merged commit 6e3a1ec into open-telemetry:main Feb 7, 2023
@RassK RassK deleted the netfx-install-gac-integrationtests branch February 8, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync production and integration tests environment for .NET Framework
4 participants