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.

Sign up for GitHub

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

#230 add in TFM's so dependencies can be optimised #231

Merged
merged 7 commits into from
Apr 21, 2024
Merged

#230 add in TFM's so dependencies can be optimised #231

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c4333d6
#230 add in TFM's so dependencies can be optimised
thompson-tomo Apr 13, 2024
48b69f5
Merge branch 'main' into chore/#230_AddTFMToOptimisedDependencies
jodydonetti Apr 20, 2024
1958b5a
Merge branch 'main' into chore/#230_AddTFMToOptimisedDependencies
jodydonetti Apr 20, 2024
59a26a0
Switch to now have a default value
thompson-tomo Apr 20, 2024
4a5b60c
Merge branch 'chore/#230_AddTFMToOptimisedDependencies' of https://gi…
thompson-tomo Apr 20, 2024
905b721
Better incoherent fail-safe max duration logic
jodydonetti Apr 21, 2024
5ac3465
Even better incoherent fail-safe max duration handling logic (less al…
jodydonetti Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...ckplane.StackExchangeRedis/ZiggyCreatures.FusionCache.Backplane.StackExchangeRedis.csproj
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why targeting net3.1 explicitly since it's not supported anymore?
Honest question, I've never multi targeted like this before so I'm trying to better understand the TFMs selection strategy here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the other comment, selecting net core 3.1 enables that framework and higher to benefit of not adding the dependency. Net 6 was for consistent but we could remove it if desired.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm looking right now at this as an inspiration and to double check.

There too I can see net6.0 in the <TargetFrameworks> element, even though below there are no conditions targeting that... I'm trying to wrap my head around this.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFrameworks>netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
<LangVersion>latest</LangVersion>
<Nullable>enable</Nullable>
<Version>1.0.0</Version>
Expand All @@ -28,6 +28,9 @@

<ItemGroup>
<PackageReference Include="StackExchange.Redis" Version="2.7.27" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Memory" Version="4.5.5" />
</ItemGroup>

Expand Down
2 changes: 1 addition & 1 deletion ...ation.CysharpMemoryPack/ZiggyCreatures.FusionCache.Serialization.CysharpMemoryPack.csproj
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the TFM should be net7 and not net6, I think, since MemoryPack targets explicitely net7 for some optimizations, see here.

Makes sense?

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>netstandard2.1;net7.0</TargetFrameworks>
<TargetFrameworks>netstandard2.1;net6.0</TargetFrameworks>
<LangVersion>latest</LangVersion>
<Nullable>enable</Nullable>
<Version>1.0.0</Version>
Expand Down
4 changes: 2 additions & 2 deletions ...rialization.SystemTextJson/ZiggyCreatures.FusionCache.Serialization.SystemTextJson.csproj
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing as above: why targeting net3.1 explicitly since it's not supported anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding net core 3.1 enables net 5 projects to also not have the dependency. Net 6 is to keep it consistent with the other location but could remove it as well,

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFrameworks>netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
<LangVersion>latest</LangVersion>
<Nullable>enable</Nullable>
<Version>1.0.0</Version>
Expand All @@ -26,7 +26,7 @@
<ProjectReference Include="..\ZiggyCreatures.FusionCache\ZiggyCreatures.FusionCache.csproj" />
</ItemGroup>

<ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Text.Json" Version="8.0.2" />
</ItemGroup>

Expand Down
3 changes: 1 addition & 2 deletions src/ZiggyCreatures.FusionCache/FusionCacheEntryOptions.cs
View file
Open in desktop
thompson-tomo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ internal DistributedCacheEntryOptions ToDistributedCacheEntryOptions(FusionCache
// PHYSICAL DURATION
TimeSpan physicalDuration;
TimeSpan durationToUse;
TimeSpan failSafeMaxDurationToUse;
TimeSpan failSafeMaxDurationToUse = DistributedCacheFailSafeMaxDuration ?? FailSafeMaxDuration;
bool incoherentFailSafeMaxDuration = false;

durationToUse = DistributedCacheDuration ?? Duration;
Expand All @@ -696,7 +696,6 @@ internal DistributedCacheEntryOptions ToDistributedCacheEntryOptions(FusionCache
}
else
{
failSafeMaxDurationToUse = DistributedCacheFailSafeMaxDuration ?? FailSafeMaxDuration;
if (failSafeMaxDurationToUse < durationToUse)
{
incoherentFailSafeMaxDuration = true;
Expand Down
8 changes: 7 additions & 1 deletion src/ZiggyCreatures.FusionCache/ZiggyCreatures.FusionCache.csproj
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above about net3.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason i did the 3 frameworks was that System.Threading.Tasks.Extensions was added in netcoreapp 3.1 however System.Diagnostics.DiagnosticSource was only added in net 6.

Copy link
Collaborator

@jodydonetti jodydonetti Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Got it, but if System.Diagnostics.DiagnosticSource was only added in net 6, why its condition is "'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'netcoreapp3.1'"?
I would've expected it has "'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'net6.0'", or am I missing something?

Again, be patient, first time dealing with these kinds of issues 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, the reason for the condition being like this is so that it only adds the dependency when it wasn't a part of the framework. If we were to add net6.0 then it would be included.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFrameworks>netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
<LangVersion>latest</LangVersion>
<Nullable>enable</Nullable>
<Version>1.0.0</Version>
Expand Down Expand Up @@ -41,7 +41,13 @@

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="8.0.0" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'netcoreapp3.1'">
<PackageReference Include="System.Diagnostics.DiagnosticSource" Version="8.0.0" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.4" />
</ItemGroup>

Expand Down
Loading