Skip to content

Conversation

@maryamariyan
Copy link
Contributor

@maryamariyan maryamariyan commented Jun 15, 2023

Summary of the changes

This is a follow up PR to #8806
The rslz project picks up the older version of System.Private.Uri. Using this fix to adds direct reference to System.Private.Uri.
This makes sure the restore steps would no longer pick up the older version (4.3.0) of System.Private.Uri.

@maryamariyan maryamariyan requested a review from a team as a code owner June 15, 2023 16:41
@DustinCampbell
Copy link
Member

cc @jaredpar as FYI

@jaredpar
Copy link
Member

This shouldn't be necessary. Generally adding a reference to System.Private.Uri is hiding a deeper issue in our dependencies. Typically a netstandard1.5 reference. What issue is driving us to make such a chnage?

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Want to understand what is driving this. Suspect this is masking a different problem

@maryamariyan
Copy link
Contributor Author

maryamariyan commented Jun 16, 2023

This PR is driving interoperability issues. I was also concerned this fix would be masking a larger issue but added the PR to get at least a little progress. This remove dependency to System.Private.Uri 4.3.0 but not the proper fix I wanted.

For example on project.asset.json file
You'd see items like:

      "System.Runtime/4.3.0": {
        "type": "package",
        "dependencies": {
          "Microsoft.NETCore.Platforms": "1.1.0",
          "Microsoft.NETCore.Targets": "1.1.0",
          "runtime.any.System.Runtime": "4.3.0"
        },
 "runtime.any.System.Runtime/4.3.0": {
        "type": "package",
        "dependencies": {
          "System.Private.Uri": "4.3.0"
        },
        "compile": {
          "ref/netstandard/_._": {}
        },
        "runtime": {
          "lib/netstandard1.5/System.Runtime.dll": {}
        }
      }
     "NETStandard.Library/1.6.1": {
        "type": "package",
        "dependencies": {
          "Microsoft.NETCore.Platforms": "1.1.0",
          "Microsoft.Win32.Primitives": "4.3.0",
          "System.AppContext": "4.3.0",
          "System.Collections": "4.3.0",
...
          "System.Runtime": "4.3.0",
        }
      },

Which confirms your thoughts about it being relevant to a netstandard 1.X reference (here seems 1.6.1)

@jaredpar
Copy link
Member

@maryamariyan and I caught up offline to dig into this.

In this case I think we have to add the System.Private.Uri reference. This is getting pulled in via Microsoft.VisualStudio.Telemetry. That does not have a .NET Core TFM so we use the netstandard2.0 TFM which has System.Diagnostics.Tracing 4.3.0 that eventually brings us the poisoned System.Private.Uri. I suspect we aren't seeing this in other components that use telemetry because they target desktop and hence get the net45 TFM which doesn't bring in System.Private.Uri.

Short term need to move forward with this. Longer term think we need to work with MS.VS.Telemetry to get a .NET Core TFM added where we can avoid this problem.

@maryamariyan maryamariyan merged commit 37ebd7f into main Jun 16, 2023
@maryamariyan maryamariyan deleted the fix-cg branch June 16, 2023 19:12
@teo-tsirpanis
Copy link

Longer term think we need to work with MS.VS.Telemetry to get a .NET Core TFM added where we can avoid this problem.

@jaredpar the dependency of the VS telemetry package to System.Diagnostics.Tracing can just be removed; it's not needed for .NET Standard 2.0.

@jaredpar
Copy link
Member

@teo-tsirpanis

the dependency of the VS telemetry package to System.Diagnostics.Tracing can just be removed; it's not needed for .NET Standard 2.0.

That is my general inclination but I've found a few cases where using netstandard2.0 has forced me to add packages like this in the past. Given that has a net451 TF though I think it makes sense to add a net6.0 independent of whether or not System.Diagnostics.Trace is removed.

@AArnott for visibility. Not sure the best place to file a suggestion / bug on MS.VS.Telemetry package.

@AArnott
Copy link

AArnott commented Jun 20, 2023

I don't see System.Private.Uri in the project.assets.json file for MS.VS.Telemetry as it is. Are you sure it is to blame for bringing in this dependency?
That said, we appear to have several package dependencies that are not needed. I remove them in this pull request.

As for adding a net6.0 target, that'll be up to the telemetry team. I see value to doing so, especially around nullable ref annotations if they wanted to turn those on.

What I don't understand is what's so bad about this System.Private.Uri reference in the first place. If you don't really need it given your TFM, such things usually compile away, don't they?

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.

7 participants