search

BcryptNet / bcrypt.net

BCrypt.Net - Bringing updates to the original bcrypt package
MIT License
836 stars 99 forks source link

Framework targeting incorrect #61

Closed jvandertil closed 4 years ago

jvandertil commented 4 years ago

Hello,

There seems to be an issue with the framework targeting for the NuGet package. If you look at the package on NuGet then it appears that there are no dependencies listed. This is strange, I expected to see System.Memory somewhere.

Also, the performance improvements (that use Span) do not seem to be part of the NuGet package. The netstandard2.1 assembly in the package still has the 'old' signatures that use arrays, as seen below in the ILdasm screenshot.

image

I believe the error is in the way the HAS_SPAN compile time constant is defined.

Currently it is:

  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0|net452|net462|net472'">
    <PackageReference Include="System.Memory" Version="4.5.3" />
  </ItemGroup>

  <PropertyGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1|netstandard2.1|netstandard2.0|net452|net462|net472'">
    <DefineConstants>$(DefineConstants);HAS_SPAN</DefineConstants>
  </PropertyGroup>

As far as I know the MSBuild '==' operator performs an exact string match.

I believe that this is the correct way to define the dependency and constant:

  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'
                         or '$(TargetFramework)' == 'net452'
                         or '$(TargetFramework)' == 'net462'
                         or '$(TargetFramework)' == 'net472'">
    <PackageReference Include="System.Memory" Version="4.5.3" />
  </ItemGroup>

  <PropertyGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1'
                         or '$(TargetFramework)' == 'netstandard2.1'
                         or '$(TargetFramework)' == 'netstandard2.0'
                         or '$(TargetFramework)' == 'net452'
                         or '$(TargetFramework)' == 'net462'
                         or '$(TargetFramework)' == 'net472'">
    <DefineConstants>$(DefineConstants);HAS_SPAN</DefineConstants>
  </PropertyGroup>
ChrisMcKee commented 4 years ago

I think the dependency graph issue in nuget is caused by the lack of a nuspec specifying the dependency. I've not really gone back to check if they changed the csproj format to allow extra fields for this or not.

Can't remember where the hell I found the target framework shortcut from but it did work when it was added 😞 . VS is now showing it grey so it seems dead.

I'm removing net47 from the targets as that should be safely covered by netstd2; I'm also reducing span coverage to netstd up.

jvandertil commented 4 years ago

Yeah, but there are only PackageReference entries. So you shouldn't need an additional nuspec to fix that. MSBuild does not understand the Condition property to be a set of 'or' conditions and thus does not output it. Same with the constants

ChrisMcKee commented 4 years ago

image Yep seems fine fixed up locally

ChrisMcKee commented 4 years ago

image so long as .net 2 stays happy we're all good for keeping enterprises up to date 😆

Thanks for that, again, I'd planned on manually testing the versions later on so good spot.

Still managed to cock up updating two files but may not be the worst thing. .net4.8 doesnt support 2.1 not wholly certain it will either 🤔

ChrisMcKee commented 4 years ago

.net 4.8 console app seems to go for the bloody 4.x ref rather than netstandard2 😱

ChrisMcKee commented 4 years ago

Sorted. god that targeting formats ugly.

ChrisMcKee commented 4 years ago

image from .net 4.8 project

jvandertil commented 4 years ago

Awesome, thanks for the quick fix 😄