-
Notifications
You must be signed in to change notification settings - Fork 305
Added support for .net 8 - DTF Service Fabric #1205
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.
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
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree company="Microsoft" |
@cgillum / @jviau - Can you please add @shankarsama as reviewer? I would appreciate a review from one of you gus as well. Also, can you please approve the ci pipeline? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bunch of package changes here that will impact all projects in this repo. Have you verified what the impact to other packages are? Keep in mind we cannot have any major version revs of any transitive dependency. This is considered a breaking change to customers and we would need to also major version rev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the other project's target .net8. All projects continue to be on the same version. Updated the Packages.props to a more understandable structure.
src/DurableTask.AzureServiceFabric/Service/.net8/FabricOrchestrationServiceController.cs
Outdated
Show resolved
Hide resolved
<PropertyGroup> | ||
<TargetFrameworks>net462;net472</TargetFrameworks> | ||
<TargetFrameworks>net8.0;net462;net472</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look fairly extensive, especially adding a new TFM. Have you evaluated what version change needs to happen? if there are any breaking changes (even revving transitive dependencies across major versions), then this package will need a major version rev as well.
These changes are very large and impactful. I think it will need to go through a breaking change review and potential major version rev. |
src/DurableTask.AzureServiceFabric/DurableTask.AzureServiceFabric.csproj
Outdated
Show resolved
Hide resolved
<PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="2.1.1" Condition="'$(TargetFramework)' != 'net8.0'" /> | ||
<PackageVersion Include="Microsoft.Extensions.Logging.Abstractions" Version="6.0.1" Condition="'$(TargetFramework)' != 'net8.0'" /> | ||
<PackageVersion Include="Microsoft.Extensions.Logging" Version="6.0.0" Condition="'$(TargetFramework)' != 'net8.0'" /> | ||
<PackageVersion Include="Microsoft.Extensions.Logging.Console" Version="6.0.0" Condition="'$(TargetFramework)' != 'net8.0'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about all these != 'net8.0'
checks, and how we will maintain these if/when we need to move to net10
in the future. Is there another way we can refactor this to make it easier to make future changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These property methods from MSBuild might do what you are asking: https://learn.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2022#msbuild-targetframework-and-targetplatform-functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jviau - Can you please elaborate? I din't follow. Do you mean using IsTargetFrameworkCompatible?
Condition="!$([MSBuild]::IsTargetFrameworkCompatible('net8.0', '$(TargetFramework)'))"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes IsTargetFrameworkCompatible
might be what you want. But I can't remember the exact behavior/syntax off the top of my head. But you should be able to use it to effectively do a >= net8.0
Added Support for .NET 8 - DTF Service Fabric Provider
Summary
This update introduces support for .NET 8 within the DTF Service Fabric Provider, alongside existing support for .NET Framework. Key changes and improvements are outlined below.
Changes
Service Structure
Controller
TaskHubProxyListener
Startup
Framework-specific Integrations
TaskHubProxyListenerBase
TaskHubProxyListenerBase
class to abstract and centralize common functionality across frameworks.Endpoint Adjustments
orchestration
orchestrationAll
RemoteOrchestrationServiceClient
to align with these changes.