Skip to content

Cleanup - Remove DotNetCoreClr #425

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

Jump to bottom
Merged
merged 6 commits into from
Jul 21, 2025
Merged

Cleanup - Remove DotNetCoreClr #425

merged 6 commits into from
Jul 21, 2025

Conversation

ilijagrbic
Copy link
Contributor

@ilijagrbic ilijagrbic commented Jun 10, 2025 •
edited
Loading

Description

  • Usage of DotNetCoreClr is a legacy thing from WindowsFabric
  • Remove it (#if DotNetCoreClr) in favour of #if NET
  • Since we use #if NET this PR requires us to build using .NET8.0, prerequisite for this is this PR

@ilijagrbic
Remove DotNetCoreClr
97f4c73
@ilijagrbic
Squashed commit of the following:
41b4a4d 
commit 26d6c61
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Tue Jun 10 13:02:24 2025 +0200

    Fix issues after develop merge

commit 6ff2942
Merge: 9f3a0a4 c4fc3a1
Author: ilijagrbic <ilijagrb.ns@gmail.com>
Date:   Tue Jun 10 12:56:21 2025 +0200

    Merge branch 'develop' into user/ilijagrbi/buildDiagnosticsWithNetCore

commit 9f3a0a4
Merge: 9e8cb39 cf6a9ce
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Wed Jun 4 10:11:16 2025 +0200

    Merge branch 'develop' into user/ilijagrbi/buildDiagnosticsWithNetCore

commit 9e8cb39
Merge: 0e47a03 9d6c4ab
Author: ilijagrbic <ilijagrb.ns@gmail.com>
Date:   Wed Jun 4 09:40:16 2025 +0200

    Merge branch 'develop' into user/ilijagrbi/buildDiagnosticsWithNetCore

commit 0e47a03
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Tue Jun 3 12:56:26 2025 +0200

    Remove unnecessary target framework

commit 9d34fab
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Tue Jun 3 10:40:25 2025 +0200

    Change NoWarn into WarningsNotAsErrors for better visibility

commit d1391e5
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Tue Jun 3 10:36:54 2025 +0200

    Fix indentation

commit d7220be
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Tue Jun 3 10:23:45 2025 +0200

    Revert "Update AssemblyVersion property to be in line with package version"

    This reverts commit 50efc5c.

commit f016fc1
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 12:27:23 2025 +0200

    Consolidate property groups

commit 0c36b5d
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 12:23:40 2025 +0200

    Remove speration for Linux/Windows build

commit 94af75b
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 12:13:41 2025 +0200

    Remove managed.props and consolodiate all props in managed_common.props

commit 8d03c48
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 12:06:27 2025 +0200

    remove not needed properties files

commit d8738fe
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 11:44:09 2025 +0200

    Remove test properties files

commit 8c5eb0a
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 11:37:10 2025 +0200

    Remove seperate Windows/Linux files

commit 50efc5c
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 11:20:14 2025 +0200

    Update AssemblyVersion property to be in line with package version

commit 451b199
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Mon Jun 2 11:16:56 2025 +0200

    Change NoWarn into WarningsNotAsErrors for better visibility

commit 8da897f
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Fri May 30 12:22:17 2025 +0200

    Remove comment

commit 7e9f610
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Fri May 30 11:41:02 2025 +0200

    Use linux props by default

commit c824509
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Fri May 30 10:37:22 2025 +0200

    Fix wrong path variable

commit 0081d8a
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Fri May 30 10:36:01 2025 +0200

    Remove not needed props

    AssemblyClsCompliant should be false since we dont use it and if removed default is false. AllowUnsafeBlocks is false by default so removing it.

commit 7a144da
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Fri May 30 10:24:21 2025 +0200

    Rename properties

commit 52bc76a
Merge: 15c8793 2b095ef
Author: ilijagrbic <ilijagrb.ns@gmail.com>
Date:   Fri May 30 10:09:55 2025 +0200

    Merge branch 'develop' into user/ilijagrbi/buildDiagnosticsWithNetCore

commit 15c8793
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Thu May 29 14:41:51 2025 +0200

    Extract common propery logic and only build Diagnostics with net8.0-windows

commit e9df3d4
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Thu May 29 13:00:07 2025 +0200

    Revert extraction of common property logic

commit 5ca6f58
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Thu May 29 12:35:44 2025 +0200

    Extract common property logic into one file

commit c751a83
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Wed May 28 16:54:15 2025 +0200

    Build seperate binary for Diagnostics project on linux

commit b54c83b
Author: Ilija Grbić <ilijagrbi@microsoft.com>
Date:   Wed May 28 16:20:29 2025 +0200

    Build all projects with net8.0-windows instead of netstandard
@ilijagrbic
Revert "Squashed commit of the following:"
62753cb 
This reverts commit 41b4a4d.
@ilijagrbic
Merge branch 'develop' into user/ilijagrbi/removeOldCompilerIfs
74ac442
@ilijagrbic
Remove the rest of DotNetCoreClrs
951f153
@ilijagrbic ilijagrbic requested a review from olegsych July 18, 2025 08:58
@ilijagrbic ilijagrbic marked this pull request as ready for review July 18, 2025 08:59
@Copilot Copilot AI review requested due to automatic review settings July 18, 2025 08:59
Copilot
Copilot AI reviewed Jul 18, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modernizes the codebase by replacing legacy DotNetCoreClr preprocessor directives with the standard NET directive, eliminating Windows Fabric legacy dependencies and standardizing on .NET 8.0+ conditional compilation.

  • Replaces all #if DotNetCoreClr with #if NET and #if !DotNetCoreClr with #if !NET
  • Removes DotNetCoreClr from build configuration properties
  • Consolidates conditional compilation logic in one instance by replacing separate #if !DotNetCoreClr and #if DotNetCoreClr blocks with #if !NET and #else

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
properties/service_fabric_managed_common.props Removes DotNetCoreClr from DefineConstants for .NET 8.0+ targets
test/unittests/Microsoft.ServiceFabric.Diagnostics.Tests/ServiceFabricEventSourceTest.cs Updates conditional compilation for test class
src/Microsoft.ServiceFabric.Services/TelemetryConstants.cs Updates OS detection conditional compilation
src/Microsoft.ServiceFabric.Services/LogContext.cs Updates logging context conditional compilation
src/Microsoft.ServiceFabric.Services/Client/ClientRequestTracker.cs Updates client request tracking conditional compilation
src/Microsoft.ServiceFabric.Services.Remoting/Builder/ProxyBase.cs Updates remoting proxy conditional compilation
src/Microsoft.ServiceFabric.Services.Remoting/Builder/CodeBuilderUtils.cs Updates assembly builder conditional compilation
src/Microsoft.ServiceFabric.Services.Remoting/Builder/CodeBuilderContext.cs Updates debugging conditional compilation
src/Microsoft.ServiceFabric.Actors/Runtime/Migration/MigrationReflectionHelper.cs Updates migration helper conditional compilation
src/Microsoft.ServiceFabric.Actors/Runtime/ActorStateProviderHelper.cs Updates state provider conditional compilation
src/Microsoft.ServiceFabric.Actors/Remoting/V2/ActorRemotingWrappingDataContractSerializationProvider.cs Updates serialization provider conditional compilation
src/Microsoft.ServiceFabric.Actors/Remoting/V2/ActorRemotingDataContractSerializationProvider.cs Updates serialization provider conditional compilation
src/Microsoft.ServiceFabric.Actors/Remoting/Runtime/ActorServiceRemotingListener.cs Updates remoting listener conditional compilation
src/Microsoft.ServiceFabric.Actors/Remoting/ActorLogicalCallContext.cs Updates logical call context conditional compilation
src/Microsoft.ServiceFabric.Actors/Remoting/ActorDataContractSurrogate.cs Updates data contract surrogate conditional compilation
src/Microsoft.ServiceFabric.Actors.KVSToRCMigration/HttpCommunicationClient.cs Updates HTTP client conditional compilation and consolidates preprocessor blocks

@ilijagrbic ilijagrbic enabled auto-merge (squash) July 18, 2025 08:59
@ilijagrbic ilijagrbic removed the request for review from olegsych July 18, 2025 15:33
@olegsych
Copy link
Member

@ilijagrbic I've approved but the PR now has merge conflicts. Since we can't approve each-other's changes I'll leave it up to you to resolve.

@ilijagrbic ilijagrbic merged commit 5faf818 into develop Jul 21, 2025
5 checks passed
@ilijagrbic ilijagrbic deleted the user/ilijagrbi/removeOldCompilerIfs branch July 21, 2025 08:17
Copilot AI pushed a commit that referenced this pull request Jul 21, 2025
@ilijagrbic @Copilot
Cleanup - Remove DotNetCoreClr (#425)
a641e2e 
**Description**

- Usage of DotNetCoreClr is a legacy thing from WindowsFabric
- Remove `#if DotNetCoreClr` in favour of `#if NET`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot

@olegsych olegsych

@plave0 plave0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ilijagrbic @olegsych @plave0