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

cleanup duplicate property implementation #10107

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

kasperk81
Copy link
Contributor

may or may not fix dotnet/installer#19667 (comment) and may or may not unblock the next one dotnet/installer#19669

@ViktorHofer @MichaelSimons @rainersigwald

@ViktorHofer
Copy link
Member

If the linker trims the msbuild assemblies either here or in the SDK layout, then I could see how this change would solve the TypeLoadException. Given that this is a positive change anyways, I would take it.

@ViktorHofer
Copy link
Member

@dotnet/kitten can you please approve and merge this PR in? We need a new SDK build with this change the sooner the better.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

If the linker trims the msbuild assemblies either here or in the SDK layout

Are SDK assemblies really trimmed? I am skeptical this change would help but it won't hurt either. In any case, it would be good to understand the root cause.

@ladipro ladipro merged commit 66dcc32 into dotnet:main May 6, 2024
10 checks passed
@rainersigwald
Copy link
Member

Are SDK assemblies really trimmed?

They are not.

@kasperk81
Copy link
Contributor Author

it did help. the next error in source-build test build fixed in #10111

@kasperk81 kasperk81 deleted the patch-2 branch May 6, 2024 19:27
@rainersigwald
Copy link
Member

@kasperk81 this change did not help. Per @ViktorHofer the problem with the source-build in installer is a build-vs-runtime split across dotnet/runtime@b8fe1d0, which will be addressed by dotnet/installer#19684.

@kasperk81
Copy link
Contributor Author

@rainersigwald, previous error was:

System.TypeLoadException: Method 'get_Count' in type 'Microsoft.Build.Collections.PropertyDictionary`1' from assembly 'Microsoft.Build, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' does not have an implementation.

next error is:

System.TypeLoadException: Method 'ContainsKey' in type 'Microsoft.Build.Collections.PropertyDictionary`1' from assembly 'Microsoft.Build, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' does not have an implementation.

so get_Count is fixed by this pr, and #10111 will fix ContainsKey.

@rainersigwald
Copy link
Member

I should rephrase: this change did not resolve the underlying problem, which was with compiling and running across an incompatible boundary in the .NET runtime.

@kasperk81
Copy link
Contributor Author

i can see that runtime change has subtle effects in multiple places and people are pleading to bring it back dotnet/runtime#31001 (comment).

@rainersigwald
Copy link
Member

Yeah, and that should be fine--it's not a breaking change (except for C++/CLI, I guess). The problem arose when we compiled targeting .NET-9-with-that-change (which I guess changed some overload resolution) and then ran on .NET-9-without-that-change. That shouldn't happen again even if it comes back (unless it's then backed out again).

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