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

Remove Microsoft.Diagnostics.Tracing.EventSource.Redist #71573

Merged
merged 3 commits into from
Jul 16, 2022

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Jul 2, 2022

This package was used historically to distribute EventSource fixes out-of-band but
is no longer needed. This change removed the project and the ES_BUILD_STANDALONE +
FEATURE_MANAGED_ETW_CHANNELS ifdefs which were only defined when the project was building.

Fixes #71231

@ghost
Copy link

ghost commented Jul 2, 2022

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

This package was used historically to distribute EventSource fixes out-of-band but
is no longer needed. This change removed the project and the ES_BUILD_STANDALONE +
FEATURE_MANAGED_ETW_CHANNELS ifdefs which were only defined when the project was building.

Fixes #71231

Author: noahfalk
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

@noahfalk
Copy link
Member Author

noahfalk commented Jul 2, 2022

@brianrob @tarekgh - PTAL

@tarekgh
Copy link
Member

tarekgh commented Jul 2, 2022

Looks the changes causing a compat issue, maybe we need to keep this API in the implementation.

Build FAILED.

/__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22327.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : Compat issues with assembly System.Diagnostics.Tracing: [/__w/1/s/src/libraries/System.Diagnostics.Tracing/src/System.Diagnostics.Tracing.csproj]
/__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22327.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : MembersMustExist : Member 'public System.Diagnostics.Tracing.EventChannel System.Diagnostics.Tracing.EventAttribute.Channel.get()' does not exist in the implementation but it does exist in the contract. [/__w/1/s/src/libraries/System.Diagnostics.Tracing/src/System.Diagnostics.Tracing.csproj]
/__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22327.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : MembersMustExist : Member 'public void System.Diagnostics.Tracing.EventAttribute.Channel.set(System.Diagnostics.Tracing.EventChannel)' does not exist in the implementation but it does exist in the contract. [/__w/1/s/src/libraries/System.Diagnostics.Tracing/src/System.Diagnostics.Tracing.csproj]
/__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22327.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : MembersMustExist : Member 'public System.Diagnostics.Tracing.EventChannel System.Diagnostics.Tracing.EventWrittenEventArgs.Channel.get()' does not exist in the implementation but it does exist in the contract. [/__w/1/s/src/libraries/System.Diagnostics.Tracing/src/System.Diagnostics.Tracing.csproj]
/__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22327.2/build/Microsoft.DotNet.ApiCompat.targets(66,5): error : ApiCompat failed for '/__w/1/s/artifacts/bin/System.Diagnostics.Tracing/Release/net7.0/System.Diagnostics.Tracing.dll' [/__w/1/s/src/libraries/System.Diagnostics.Tracing/src/System.Diagnostics.Tracing.csproj]
    0 Warning(s)
    5 Error(s)

@tarekgh
Copy link
Member

tarekgh commented Jul 2, 2022

CC @ericstj for awareness.

@ericstj
Copy link
Member

ericstj commented Jul 5, 2022

CC @ericstj for awareness.

I am OK with the premise of this PR, but it needs to be done in a way that doesn't break the package/API we still ship.

@noahfalk
Copy link
Member Author

noahfalk commented Jul 5, 2022

I am OK with the premise of this PR, but it needs to be done in a way that doesn't break the package/API we still ship.

For sure. I think it was a mistake on the FEATURE_MANAGED_ETW_CHANNELS which was defined elsewhere and the local builds I had done didn't have the right configurations to spot it.

@ericstj
Copy link
Member

ericstj commented Jul 5, 2022

You should be able to repro locally with a build of libs, then dotnet build of src/libraries/System.Diagnostics.Tracing/src/System.Diagnostics.Tracing.csproj which should build everything with minimal overhead.

…t-of-band but

is no longer needed. This change removed the project and the ES_BUILD_STANDALONE
ifdef which was only defined when the project was building.

Fixes dotnet#71231
@noahfalk
Copy link
Member Author

noahfalk commented Jul 7, 2022

Build failure is known issue: #71684

@noahfalk
Copy link
Member Author

noahfalk commented Jul 7, 2022

I think all the feedback has been addressed, does this look good now? Thanks!

Copy link
Member

@brianrob brianrob left a comment

Choose a reason for hiding this comment

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

LGTM!

@noahfalk
Copy link
Member Author

Thanks all! I'm merging on red but the issue shown by build analysis is a known one unrelated to this change: #71684

@noahfalk noahfalk merged commit b6932fd into dotnet:main Jul 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.Diagnostics.Tracing.EventSource.Redist package support will end with .NET 6
5 participants