Use HashCode instead of HashHelpers in netstandard libs#41165
Use HashCode instead of HashHelpers in netstandard libs#41165stephentoub wants to merge 1 commit intodotnet:masterfrom
Conversation
| UnmanagedType marshalAsType = GetMarshalAsParameterValue(index); | ||
| return marshalAsType == UnmanagedType.Interface || marshalAsType == UnmanagedType.IUnknown || marshalAsType == UnmanagedType.IDispatch; | ||
| return marshalAsType == UnmanagedType.Interface || marshalAsType == UnmanagedType.IUnknown || | ||
| #pragma warning disable CS0618 // obsolete |
There was a problem hiding this comment.
@terrajobst, @AaronRobinsonMSFT, this surprised me. UnmanagedType.IDispatch is not obsolete in netstandard2.0 nor netcoreapp3.0, but is in netstandard2.1. Was that an explicit choice?
2e4686c to
f3994c0
Compare
| <SupportedFramework>uap10.0.16299;net461;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework> | ||
| </ProjectReference> | ||
| <ProjectReference Include="..\src\System.Resources.Extensions.csproj" /> | ||
| <HarvestIncludePaths Include="lib/netstandard2.0" /> |
There was a problem hiding this comment.
This won’t work until we have a stable 3.0 to harvest from.
There was a problem hiding this comment.
Thanks, @ericstj. It's otherwise correct, and we just need to sit on this briefly until it'll work?
There was a problem hiding this comment.
You might need to lock the reference assembly version down if it applies to netstandard2.0. You can't have higher version ref than harvested lib. Package testing will fail if this is the case.
There was a problem hiding this comment.
I'm a bit nervous about eliminating the ns2.0 build from master, since MSBuild needs to consume this build for ResGen. If we harvest that means we don't have a way (other than servicing) to deliver changes for MSbuild to consume. I don't think I want to do this. Can we use shared source instead? Apologies for not raising this earlier.
There was a problem hiding this comment.
Can we use shared source instead?
You mean use HashCode.cs from shared source instead of HashHelpers from shared source?
There was a problem hiding this comment.
I'm ok with that, just need a decent hashing method. We don't want a new assembly dependency because MSBuild -> VisualStudio will notice (assuming no one is bringing Microsoft.BCL.HashCode already).
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
HashHelpers -> HashCode conversion LGTM.
Three remaining libraries were using HashHelpers.cs. Move them to netstandard2.1, change to use HashCode, and delete HashHelpers.cs.
f3994c0 to
c56b23f
Compare
| <Project> | ||
| <PropertyGroup> | ||
| <BuildConfigurations> | ||
| netstandard; |
There was a problem hiding this comment.
Given the build error, it seems like we still need to support netstandard(2.0) in this project. We could harvest the netstandard2.0 assets into the package, or use ifdefs for this code and use HashHelpers whenever TargetGroup == netstandard -- @ericstj thoughts on harvesting? It seems like this package is compat package from core to desktop and that we don't plan to add new API to it and like we don't service it much.
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <Configurations>netstandard-Debug;netstandard-Release</Configurations> | ||
| <DefineConstants>$(DefineConstants);RESOURCES_EXTENSIONS;INTERNAL_NULLABLE_ATTRIBUTES</DefineConstants> | ||
| <DefineConstants>$(DefineConstants);RESOURCES_EXTENSIONS;netstandard</DefineConstants> |
There was a problem hiding this comment.
Why is the netstandard define needed here?
|
I'm just going to close this. We can address it if/when we ever update these packages. |
Three remaining libraries were using HashHelpers.cs. Move them to netstandard2.1, change to use HashCode, and delete HashHelpers.cs.