BcryptNet/bcrypt.net

Framework targeting incorrect

jvandertil opened this issue · 8 comments

jvandertil commented

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

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

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

image
Yep seems fine fixed up locally

ChrisMcKee commented

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

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

ChrisMcKee commented

Sorted. god that targeting formats ugly.

ChrisMcKee commented

image
from .net 4.8 project

jvandertil commented

Awesome, thanks for the quick fix 😄

  • 👍1