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

change servicehub project target to net6.0-windows #59341

Closed
wants to merge 1 commit into from

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Feb 7, 2022

Fixes build errors with main and latest VS, similar to
#59324

Not sure if this is correct

cc @genlu

@dibarbet dibarbet requested a review from a team as a code owner February 7, 2022 19:06
@dibarbet dibarbet changed the title change target to net6.0-windows change servicehub project target to net6.0-windows Feb 7, 2022
@@ -4,7 +4,7 @@
<PropertyGroup>
<OutputType>Library</OutputType>
<RootNamespace>Microsoft.CodeAnalysis.Remote</RootNamespace>
<TargetFrameworks>net6.0;netstandard2.0</TargetFrameworks>
<TargetFrameworks>net6.0-windows;netstandard2.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Don't we use this (or have plans to use this) on VS for Mac too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume yes, but not sure how far along those plans are and if this is currently only expected to target windows based on its current dependencies

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's necessary to target our remote projects to net6 instead of netcoreapp3.1 (other than the CoreComponent project, which is used to pull in dependencies for .net 6 host in VS)

@CyrusNajmabadi
Copy link
Member

I think we need @tmat to weigh in. This seems... bad. This project shouldn't have a windows dependency.

@dibarbet
Copy link
Member Author

dibarbet commented Feb 7, 2022

From the binlog, it looks like there are various dependencies on Ms.Vs.Utilities, which has this 'frameworkReferences item indicating it needs MS.WindowsDesktop.App which I think is causing the build to say it needs the -windows flag

"Microsoft.VisualStudio.Utilities/17.0.0-previews-1-31410-258": {
        "type": "package",
        "dependencies": {
          "Microsoft.ServiceHub.Client": "2.8.2019",
          "Microsoft.VisualStudio.RpcContracts": "17.0.50-preview-0001-0001",
          "Microsoft.VisualStudio.Telemetry": "16.3.176",
          "System.ComponentModel.Composition": "4.5.0",
          "System.Threading.AccessControl": "5.0.0",
          "System.Threading.Tasks.Dataflow": "5.0.0"
        },
        "compile": {
          "lib/netcoreapp3.1/Microsoft.VisualStudio.Utilities.dll": {}
        },
        "runtime": {
          "lib/netcoreapp3.1/Microsoft.VisualStudio.Utilities.dll": {}
        },
        "frameworkReferences": [
          "Microsoft.WindowsDesktop.App"
        ],

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

This project is not expected to be referencing WPF or Windows Forms. Can you clarify the specific dependency which is bringing this in?

@sharwell
Copy link
Member

sharwell commented Feb 7, 2022

Microsoft.VisualStudio.Utilities/17.0.0-previews-1-31410-258

This is an outdated dependency. Does the same occur for 17.0.31723.112?

@tmat
Copy link
Member

tmat commented Feb 7, 2022

This PR will remove dependency on EditorFeatures: #59316. Is that what's referencing Microsoft.VisualStudio.Utilities?

@sharwell
Copy link
Member

sharwell commented Feb 7, 2022

@tmat EditorFeatures also doesn't have a WPF dependency, so even if we reference EditorFeatures we should not be having a problem here.

@tmat
Copy link
Member

tmat commented Feb 7, 2022

Yes, I wouldn't expect that. Just checking.

@dibarbet
Copy link
Member Author

dibarbet commented Feb 7, 2022

@tmat @sharwell ah I think removing editorfeatures will work. MS.VS.Utilities is coming in via the LSP client which is referenced via editorfeatures

"Microsoft.VisualStudio.LanguageServer.Client/17.0.5139-g7b8c8bd49d": {
        "type": "package",
        "dependencies": {
          "Microsoft.VisualStudio.CoreUtility": "17.0.323-preview",
          "Microsoft.VisualStudio.Shell.15.0": "17.0.0-previews-1-31410-258",
          "Microsoft.VisualStudio.Utilities": "17.0.0-previews-1-31410-258",
          "Microsoft.VisualStudio.Validation": "17.0.16-alpha",
          "StreamJsonRpc": "2.8.21"
        },

I will test out Tomas' PR and verify if it works.

as a side note, @gundermanc how far along is the buildable LSP client on mac? I would expect if the client is working on vsmac then we shouldn't need to target .net6.0-windows for it (or you'd have hit the same error with MS.VS.Utilities)

@jmarolf
Copy link
Contributor

jmarolf commented Feb 7, 2022

Microsoft.VisualStudio.Utilities/17.0.0-previews-1-31410-258

This is an outdated dependency. Does the same occur for 17.0.31723.112?

Yes, the same error is present. Microsoft.VisualStudio.Utilities will need to push a new package that does not take a dependency on windows only libraries and push a new package

@sharwell
Copy link
Member

sharwell commented Feb 7, 2022

Yes, the same error is present. Microsoft.VisualStudio.Utilities will need to push a new package that does not take a dependency on windows only libraries and push a new package

Can we either disable the new warning, or force the build to reference the netstandard2.0 TFM from this package instead of the netcoreapp3.1 TFM? The netstandard2.0 target is compatible with net6.0 even if netcoreapp3.1 is not.

@gundermanc
Copy link
Member

gundermanc commented Feb 7, 2022

as a side note, @gundermanc how far along is the buildable LSP client on mac?

About 5 of the features work already. We're planning to finish at least 11 of them in 17.2 timeframe.

I would expect if the client is working on vsmac then we shouldn't need to target .net6.0-windows for it

Microsoft.VisualStudio.LanguageServer.Client.dll unfortunately has a dependency on Microsoft.VisualStudio.Shell.15.0 IIRC solely for implementing a VS Package attribute. Though ugly, this dependency isn't used for anything, so I wouldn't expect it to cause problems in typical usage. What's the error?

FWIW the latest 17.2 version of the package also has a Netstandard2.0 version of the DLL without this dependency.

The long term goal is to eliminate this dependency, but it's a breaking change to public API.

@dibarbet
Copy link
Member Author

dibarbet commented Feb 7, 2022

@tmat your change fixes the issue for me locally as well.

Microsoft.VisualStudio.LanguageServer.Client.dll unfortunately has a dependency on Microsoft.VisualStudio.Shell.15.0 IIRC solely for implementing a VS Package attribute. Though ugly, this dependency isn't used for anything, so I wouldn't expect it to cause problems in typical usage. What's the error?

@gundermanc I think there was a nuget change that started checking transitive dependencies for windows dependencies. So MS.VS.Utilities declares a frameworkReference to "Microsoft.WindowsDesktop.App" and so gives build errors in any project referencing it (transitively) when the target framework is net6.0 (it wants us to target net6.0-windows).

@gundermanc
Copy link
Member

So MS.VS.Utilities declares a frameworkReference to "Microsoft.WindowsDesktop.App" and so gives build errors in any project referencing it (transitively) when the target framework is net6.0 (it wants us to target net6.0-windows).

My understanding is that there is a version of MS.VS.Utilities that is more Mac friendly. Not sure if it's a separate package or just a different TFM. @KirillOsenkov or @sandyarmstrong might know.

@sandyarmstrong
Copy link
Member

sandyarmstrong commented Feb 7, 2022

So MS.VS.Utilities declares a frameworkReference to "Microsoft.WindowsDesktop.App" and so gives build errors in any project referencing it (transitively) when the target framework is net6.0 (it wants us to target net6.0-windows).

My understanding is that there is a version of MS.VS.Utilities that is more Mac friendly. Not sure if it's a separate package or just a different TFM. @KirillOsenkov or @sandyarmstrong might know.

The MS.VS.Utilities package has net472, netstandard20, and netcoreapp31 builds in the same nupkg. VSMac depends on the netstandard20 build. I believe this resolves automatically for us due to our net6.0-macos TFM.

@dibarbet
Copy link
Member Author

dibarbet commented Feb 7, 2022

closing in favor of #59342

@dibarbet dibarbet closed this Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants