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

fix: Set Fallback for dotnetcore3.1 or net5.0 applications #1111

Merged
merged 2 commits into from
Jan 9, 2024
Merged

fix: Set Fallback for dotnetcore3.1 or net5.0 applications #1111

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
526fbb6
fix: Set Fallback for dotnetcore3.1 or net5.0 applications
samtrion Jan 3, 2024
65f99f8
Merge branch 'main' into main
shocklateboy92 Jan 9, 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
1 change: 1 addition & 0 deletions Src/CSharpier.MsBuild/build/CSharpier.MsBuild.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<CSharpier_FrameworkVersion Condition="'$(CSharpier_FrameworkVersion)' == '' and $([MSBuild]::VersionGreaterThanOrEquals($(NETCoreSdkVersion), '8.0'))">net8.0</CSharpier_FrameworkVersion>
<CSharpier_FrameworkVersion Condition="'$(CSharpier_FrameworkVersion)' == '' and $([MSBuild]::VersionGreaterThanOrEquals($(NETCoreSdkVersion), '7.0'))">net7.0</CSharpier_FrameworkVersion>
<CSharpier_FrameworkVersion Condition="'$(CSharpier_FrameworkVersion)' == '' and $([MSBuild]::VersionGreaterThanOrEquals($(NETCoreSdkVersion), '6.0'))">net6.0</CSharpier_FrameworkVersion>
<CSharpier_FrameworkVersion Condition="'$(CSharpier_FrameworkVersion)' == ''" Label="Fallback">net8.0</CSharpier_FrameworkVersion>
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, .net6 is the latest version that's still supported:
https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core

This means if your project targets an EOL'd version of dotnet, you need to also have dotnet 8 runtime installed on your machine.
While this is a better fallback than just crashing with a path not found, I wonder if it's possible to log a warning here? Or at least update the troubleshooting docs so people know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are enough projects that must remain on the 'netstandard2.0'. Such as Analyzer, Source Generators and many other project types, which of course also crash, unintentionally. Hence the fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition, .NET 8 is the new LTS, so it is always installed on every system with the current VS version. This also means the maximum support period until we have to talk about a new fallback.

Copy link
Owner

Choose a reason for hiding this comment

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

My understanding of $(NETCoreSdkVersion) is that it is based on the version of the sdk that is performing the build. The current version of this does work when targeting netstandard2.0 as long as it is being built with net6-8.

There is a set of tests using docker that validate that - https://github.com/belav/csharpier/blob/main/Tests/MsBuild/Scenarios.json

Out of curiosity I updated them to run with net5 and netcore3.1 - #1112
They both fail with this type of message

  #10 2.657   Could not execute because the specified command or file was not found.
  #10 2.657   Possible reasons for this include:
  #10 2.657     * You misspelled a built-in dotnet command.
  #10 2.658     * You intended to execute a .NET program, but dotnet-/root/.nuget/packages/csharpier.msbuild/0.0.1/build/../tools/csharpier//dotnet-csharpier.dll does not exist.

So it does make sense that this should include a fallback or at least generate a more useful error/warning telling the user the problem.

Does a global.json that specifies an older version like net5 result in $(NETCoreSdkVersion) being that same net5 version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding of $(NETCoreSdkVersion) is that it is based on the version of the sdk that is performing the build. The current version of this does work when targeting netstandard2.0 as long as it is being built with net6-8.

Okay, chances are people doing development will probably have at least one supported version of .net installed on their machine, so this is a good fallback in that case.

I'll merge this as is. @belav if you think having a better error message is worthwhile, we can track that as a separate issue.

<CSharpierDllPath>$(MSBuildThisFileDirectory)../tools/csharpier/$(CSharpier_FrameworkVersion)/dotnet-csharpier.dll</CSharpierDllPath>
<CSharpierArgs Condition="'$(CSharpier_Check)' == 'true'">$(CSharpierArgs) --check</CSharpierArgs>
<CSharpierArgs Condition="'$(CSharpier_LogLevel)' != ''">$(CSharpierArgs) --loglevel $(CSharpier_LogLevel)</CSharpierArgs>
Expand Down
Loading