Skip to content

Fix runtime detection in targets file #477

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

Merged
merged 8 commits into from
May 23, 2025

Conversation

jamescourtney
Copy link
Owner

Fix for #476

@jamescourtney jamescourtney requested a review from Copilot May 23, 2025 08:09
Copy link
Contributor

@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

A fix to ensure the .targets file correctly detects installed .NET SDK versions by capturing and parsing the dotnet --list-sdks output.

  • Enabled MSBuild capture of SDK list output (StdOut) for detection
  • Consolidated version checks into a single PropertyGroup with IsMatchingX flags and conditional overrides
  • Removed legacy version-specific groups and the FlatSharpPrettyPrint flag handling, and added a diagnostic message
Comments suppressed due to low confidence (2)

src/FlatSharp.Compiler/FlatSharp.Compiler.targets:174

  • Removing the FlatSharpPrettyPrint property group disables the --pretty-print option for the compiler; reintroduce conditional handling or merge it into the unified CompilerCommand construction.
<PropertyGroup Condition=" '$(FlatSharpPrettyPrint)' == 'true' ">

src/FlatSharp.Compiler/FlatSharp.Compiler.targets:103

  • [nitpick] Hard-coding version checks for each major release requires manual updates for .NET 10+; consider extracting the highest major version dynamically to reduce future maintenance.
<IsMatching6>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '6\\.0\\.\d+'))</IsMatching6>

Copy link

codecov bot commented May 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.04%. Comparing base (dd15bb6) to head (0c29611).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #477   +/-   ##
=======================================
  Coverage   96.04%   96.04%           
=======================================
  Files         124      124           
  Lines        8928     8928           
  Branches      749      749           
=======================================
  Hits         8575     8575           
  Misses        261      261           
  Partials       92       92           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd15bb6...0c29611. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jamescourtney jamescourtney requested a review from Copilot May 23, 2025 08:45
Copy link
Contributor

@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 refactors how the FlatSharp compiler MSBuild targets detect the .NET version and cleans up CI workflow configurations.

  • Switches from dotnet --list-sdks to dotnet --version for version detection
  • Consolidates CompilerVersion assignments into one PropertyGroup with conditional overrides for .NET 6–9
  • Removes the deprecated dotnet-quality input from CI workflow files

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/FlatSharp.Compiler/FlatSharp.Compiler.targets Updated runtime detection logic; refactored CompilerVersion settings; added a diagnostic message
.github/workflows/stryker.yml Removed deprecated dotnet-quality input
.github/workflows/dotnet.yml Removed deprecated dotnet-quality input
.github/workflows/codecov.yml Removed deprecated dotnet-quality input
Comments suppressed due to low confidence (1)

src/FlatSharp.Compiler/FlatSharp.Compiler.targets:107

  • [nitpick] Setting CompilerVersion to net9.0 by default makes the explicit condition for 9.0 redundant. You could remove the 9-case or adjust the default placement for clarity.
<CompilerVersion>net9.0</CompilerVersion>

Comment on lines 98 to 106
<Exec Command="dotnet --version" ConsoleToMsBuild="true">
<Output TaskParameter="ConsoleOutput" PropertyName="StdOut" />
</Exec>

<PropertyGroup>
<CompilerVersion>net9.0</CompilerVersion>
</PropertyGroup>

<!-- try .net6.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '6\.0\.\d+')) ">
<CompilerVersion>net6.0</CompilerVersion>
</PropertyGroup>

<!-- try .net7.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '7\.0\.\d+')) ">
<CompilerVersion>net7.0</CompilerVersion>
</PropertyGroup>

<!-- try .net8.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '8\.0\.\d+')) ">
<CompilerVersion>net8.0</CompilerVersion>
</PropertyGroup>

<!-- try .net9.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '9\.0\.\d+')) ">
<CompilerVersion>net9.0</CompilerVersion>
</PropertyGroup>
<IsMatching6>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '6\.0\.\d+'))</IsMatching6>
<IsMatching7>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '7\.0\.\d+'))</IsMatching7>
<IsMatching8>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '8\.0\.\d+'))</IsMatching8>
<IsMatching9>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '9\.0\.\d+'))</IsMatching9>
Copy link
Preview

Copilot AI May 23, 2025

Choose a reason for hiding this comment

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

Using dotnet --version reports the SDK version, not installed runtimes. To detect runtimes, consider using dotnet --list-runtimes instead.

Suggested change
<Exec Command="dotnet --version" ConsoleToMsBuild="true">
<Output TaskParameter="ConsoleOutput" PropertyName="StdOut" />
</Exec>
<PropertyGroup>
<CompilerVersion>net9.0</CompilerVersion>
</PropertyGroup>
<!-- try .net6.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '6\.0\.\d+')) ">
<CompilerVersion>net6.0</CompilerVersion>
</PropertyGroup>
<!-- try .net7.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '7\.0\.\d+')) ">
<CompilerVersion>net7.0</CompilerVersion>
</PropertyGroup>
<!-- try .net8.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '8\.0\.\d+')) ">
<CompilerVersion>net8.0</CompilerVersion>
</PropertyGroup>
<!-- try .net9.0. -->
<PropertyGroup Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '9\.0\.\d+')) ">
<CompilerVersion>net9.0</CompilerVersion>
</PropertyGroup>
<IsMatching6>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '6\.0\.\d+'))</IsMatching6>
<IsMatching7>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '7\.0\.\d+'))</IsMatching7>
<IsMatching8>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '8\.0\.\d+'))</IsMatching8>
<IsMatching9>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), '9\.0\.\d+'))</IsMatching9>
<Exec Command="dotnet --list-runtimes" ConsoleToMsBuild="true">
<Output TaskParameter="ConsoleOutput" PropertyName="StdOut" />
</Exec>
<PropertyGroup>
<IsMatching6>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), 'Microsoft\.NETCore\.App 6\.0\.\d+'))</IsMatching6>
<IsMatching7>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), 'Microsoft\.NETCore\.App 7\.0\.\d+'))</IsMatching7>
<IsMatching8>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), 'Microsoft\.NETCore\.App 8\.0\.\d+'))</IsMatching8>
<IsMatching9>$([System.Text.RegularExpressions.Regex]::IsMatch($(StdOut), 'Microsoft\.NETCore\.App 9\.0\.\d+'))</IsMatching9>

Copilot uses AI. Check for mistakes.

@jamescourtney jamescourtney merged commit 455badd into main May 23, 2025
8 checks passed
@jamescourtney jamescourtney deleted the fix_targets_file_runtime_detection branch May 23, 2025 09:02
jamescourtney added a commit that referenced this pull request May 23, 2025
* Fix runtime detection in targets file

* Restore pretty print

* Use --version instead of --list-sdks

* Update dotnet.yml

* Update codecov.yml

* Update stryker.yml

* copilot can't make up its mind
jamescourtney added a commit that referenced this pull request May 26, 2025
* Fix runtime detection in targets file

* Restore pretty print

* Use --version instead of --list-sdks

* Update dotnet.yml

* Update codecov.yml

* Update stryker.yml

* copilot can't make up its mind
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant