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

Use ComHost generation code from HostModel. #3884

Merged
merged 13 commits into from
Mar 6, 2020

Conversation

jkoritzinsky
Copy link
Member

Now that the ComHost generation code exists in Microsoft.NET.HostModel, change the sdk to use that implementation instead of the one in the SDK itself.

cc: @nguerrera

@nguerrera
Copy link
Contributor

nguerrera commented Nov 12, 2019

The reason why I didn't want to take System.Text.Json in a fast approaching release in a nuthsell:

   F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'System.Memory, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified. [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018: File name: 'System.Memory, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at System.Text.Json.JsonSerializer.WriteCoreString(Object value, Type type, JsonSerializerOptions options) [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options) [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at Microsoft.NET.HostModel.ComHost.ClsidMap.Create(MetadataReader metadataReader, String clsidMapPath) [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at Microsoft.NET.Build.Tasks.GenerateClsidMap.ExecuteCore() [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at Microsoft.NET.Build.Tasks.TaskBase.Execute() [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]
      F:\workspace\_work\1\s\artifacts\bin\Release\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(448,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext() [F:\workspace\_work\1\s\artifacts\tmp\Release\It_copies_the---095E2160\ComServer.csproj]

@nguerrera
Copy link
Contributor

This is going to come down to the age old issue of binding redirects being needed on framework vs. msbuild tasks which can't provide them, I think. There's a ref to version 4.0.1.0 but I think we have 4.0.1.1.

MSBuild itself actually depends on this, but does not have a binding redirect for it.

cc @rainersigwald @ericstj

@nguerrera
Copy link
Contributor

nguerrera commented Nov 12, 2019

@ericstj I don't suppose we can ask for System.Text.Json out of master to have a ref graph with at most one version per assembly, like we managed to arrange for MetadataLoadContext. I think at this point, with this targeting 5.0, I'd prefer to look for a more sustainable solution.

@jkoritzinsky
Copy link
Member Author

@ericstj any chance you can help out with Nick's question?

@ericstj
Copy link
Member

ericstj commented Jan 10, 2020

Sorry I missed this. Could be possible, depends on what we need to target. I'll file an issue.

@ericstj
Copy link
Member

ericstj commented Jan 10, 2020

Actually, If I install the latest System.Text.Json it references System.Memory 4.0.1.1 so we may have already fixed this. Can you double check @nguerrera?

@jkoritzinsky
Copy link
Member Author

It looks like Microsoft.NET.HostModel is still compiling against an older System.Text.Json. The subscriptions in dotnet/runtime haven't been updated, and as a result the reference is out of date. I'll open an issue on dotnet/runtime about it.

@jkoritzinsky
Copy link
Member Author

@ericstj I just built System.Text.Json locally and I don't see it referencing System.Memory 4.0.1.1, I only see it referencing 4.0.1.0. Where are you seeing the updated version reference?

@ericstj
Copy link
Member

ericstj commented Jan 23, 2020

I looked at the last shipping package.

// C:\Users\ericstj\.nuget\packages\system.text.json\4.7.0\lib\netstandard2.0\System.Text.Json.dll
// System.Memory, Version=4.0.1.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

Indeed if I look at the state of dotnet/runtime I confirm what you're seeing. Something regressed this post 3.x. I'll see what's going on.

@jkoritzinsky
Copy link
Member Author

I just tested this PR with a HostModel built against System.Text.Json 4.7.0 and it worked, so I'm going to go with that for now while we try to figure out what regressed post 3.x.

@ericstj
Copy link
Member

ericstj commented Jan 23, 2020

I found the regression and put up a PR to fix it.

@dsplaisted
Copy link
Member

@jkoritzinsky @ericstj What's the status of this?

@jkoritzinsky
Copy link
Member Author

It’s blocked on dotnet/runtime#2115 which is blocked on dotnet/runtime#1918

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Mar 5, 2020

Finally, this PR is green! @nguerrera @wli3 any other feedback? I'll merge this in by EOD if I don't hear anything.

@jkoritzinsky jkoritzinsky merged commit fabf29b into dotnet:master Mar 6, 2020
@jkoritzinsky jkoritzinsky deleted the use-hostmodel-comhost branch March 6, 2020 01:56
@nguerrera
Copy link
Contributor

Yay!

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.

4 participants