[Core] Upgrading System.Diagnostics.DiagnosticSource to 5.0.0 (netcoreapp2.1)/ 6.0.1
#37198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
API change check API changes are not detected in this pull request. |
m-redding
changed the title
m-redding
changed the title
System.Diagnostics.DiagnosticSource to 5.0.0 (netcoreapp2.1)/ 6.0.1
m-redding
requested review from
hallipr,
weshaggard,
benbp,
JoshLove-msft,
christothes,
annelo-msft and
KrzysztofCwalina
as code owners
weshaggard
reviewed
Jun 26, 2023
christothes
approved these changes
Jun 27, 2023
|
Were we able to talk to someone from PowerShell to confirm that this change is okay for them? |
| @@ -68,7 +68,7 @@ | |||
| <PackageReference Update="System.Memory.Data" Version="1.0.2" /> | |||
| <PackageReference Update="System.Numerics.Vectors" Version="4.5.0" /> | |||
| <PackageReference Update="System.Net.Http" Version="4.3.4" /> | |||
| <PackageReference Update="System.Diagnostics.DiagnosticSource" Version="4.6.0" /> | |||
| <PackageReference Update="System.Diagnostics.DiagnosticSource" Version="6.0.1" /> | |||
There was a problem hiding this comment.
How did we land on this version vs 5.x?
There was a problem hiding this comment.
5.x is deprecated, so I don't think it would be ideal to take a dependency on it. 6.0.1 is the recommended 6.x version
I was able to touch base with someone on their team and they weren't aware of any issues that would arise from this change. The newest version of PowerShell 7 takes a dependency on |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first piece of resolving #33683. Unfortunately, given the dependencies between packages, I don't think it's possible to upgrade the dependency and use the new types in the same release, or at the very least in the same PR. I think we may need to release with the upgraded dependency first and then merge in the rest of the work in this PR: #37149.
Initially, there were concerns about PowerShell and functions integrations when upgrading this dependency. After doing some research, the most recent versions of PowerShell 7 depend on the newest version of the
System.Diagnostics.DiagnosticSource(7.*), so upgrading this dependency should not impact PowerShell integration.In order to support both netcoreapp2.1 and the new types, we need to multitarget with 5.0.0.
System.Diagnostics.DiagnosticSource6.0.1 has a transitive dependency that does not support netcoreapp2.1. However,System.Diagnostics.DiagnosticSource5.0.0 is deprecated, so we shouldn't take a dependency on that version anywhere else.There are also some changes here to
MessagingClientDiagnostics. This file was causing pipeline errors that needed some simple updates to fix.This also contributes to: #35572
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.