Skip to content

Conversation

taooceros
Copy link
Member

@taooceros taooceros commented Aug 13, 2021

#486
We cannot bump squirrel higher than 1.9.0

@taooceros
Copy link
Member Author

I didn't find some issue currently. Though, our pinyin library has updated its data source, which seems significantly increase the memory usage.

@jjw24
Copy link
Member

jjw24 commented Aug 14, 2021

our pinyin library has updated its data source, which seems significantly increase the memory usage

cpu and memory usage is important, so if the library increases memory, let's not bump it.

<PackageReference Include="ToolGood.Words.Pinyin" Version="3.0.1.4" />
<PackageReference Include="NLog.Web.AspNetCore" Version="4.13.0" />
<PackageReference Include="System.Drawing.Common" Version="5.0.2" />
<PackageReference Include="ToolGood.Words.Pinyin" Version="3.0.2.6" />
Copy link
Member

Choose a reason for hiding this comment

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

should we not bump this then

@jjw24 jjw24 added the enhancement New feature or request label Aug 14, 2021
@jjw24 jjw24 added this to the 1.9.0 milestone Aug 14, 2021
@taooceros
Copy link
Member Author

taooceros commented Aug 15, 2021

The memory inflation will only appear when people use pinyin, but the new version can also increase the accuracy of pinyin, which probably is acceptable.

@taooceros taooceros requested a review from jjw24 August 19, 2021 03:22
@jjw24
Copy link
Member

jjw24 commented Aug 19, 2021

downgrade pinyin library until further discussion

Thanks, i was about to suggest to pull this out as a seperate PR. Did you find more issues from this bump?

@jjw24 jjw24 changed the title Bump Dependency Bump applicable nuget packages to latest versions Aug 19, 2021
@jjw24
Copy link
Member

jjw24 commented Aug 19, 2021

havent had the time to look into this, we are getting error with the build:
image

@jjw24 jjw24 added the review in progress Indicates that a review is in progress for this PR label Aug 23, 2021
@jjw24
Copy link
Member

jjw24 commented Aug 24, 2021

Ok so this error is coming with nuget pack version > 5.8.1. This is part of the wider .net 5 sdk change now requiring you to specify platform version e.g. net5.0-windowsX.Y (X.Y as TPV- TargetPlatformVersion). The target framework net5.0-windows for some of our projects is still valid in the csproj files even though we are not specifying which version of Windows because the .NET SDK provides a default, and NuGet uses that to generate lib/net5.0-windows7.0/your.dll. Since we are not packing via csproj, we wont get the message from there, but we are using command-line to pack so need to append the platform version number to our nuspec.

I am going to separate out this issue so I can isolate the testing for this as I also want to try appending the target platform version to the csproj files.

nuget pack 5.10.0 requires target platform version to be appended to nuspec
jjw24
jjw24 previously approved these changes Aug 24, 2021
@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Aug 24, 2021
@jjw24 jjw24 merged commit 03166ec into dev Sep 23, 2021
@jjw24 jjw24 deleted the DependencyBump branch September 23, 2021 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants