NightOwl888 commented on code in PR #1222:
URL: https://github.com/apache/lucenenet/pull/1222#discussion_r2653224168
##########
Directory.Build.props:
##########
@@ -31,6 +31,19 @@
<GitHubProject>lucenenet</GitHubProject>
</PropertyGroup>
+ <PropertyGroup Label="Centralized Target Frameworks">
Review Comment:
Let's centralize the legacy Visual Studio logic here. This will likely be
something we need to deal with again when future versions of Visual Studio are
released. Here is how this was done in J2N:
```xml
<PropertyGroup Label="Legacy Visual Studio Support">
<VisualStudioMajorVersion>0</VisualStudioMajorVersion>
<!-- Extract major version (works for preview builds too) -->
<VisualStudioMajorVersion Condition="'$(VisualStudioVersion)' !=
''">$([System.Version]::Parse($([System.Text.RegularExpressions.Regex]::Match('$(VisualStudioVersion)',
'^\d+(\.\d+)*').Value)).Major)</VisualStudioMajorVersion>
<IsLegacyVisualStudioVersion>false</IsLegacyVisualStudioVersion>
<!-- Visual Studio 2026 == Version 18.x - anything below that major
version is legacy -->
<IsLegacyVisualStudioVersion Condition="'$(BuildingInsideVisualStudio)'
== 'true' And '$(VisualStudioMajorVersion)' <
'18'">true</IsLegacyVisualStudioVersion>
</PropertyGroup>
```
This gives us a simple boolean to be used elsewhere instead of repeating
this logic in other places (like `Lucene.Net.Analysis.OpenNLP`).
With that in place, the below can be simplified to:
```xml
<PropertyGroup Label="Centralized Target Frameworks">
<!-- Add net10.0 conditionally based on VS version (VS2024+ = 18.0) -->
<LibraryTargetFrameworks>net10.0;net8.0;netstandard2.0;net462</LibraryTargetFrameworks>
<LibraryTargetFrameworks Condition=" '$(IsLegacyVisualStudioVersion)' ==
'true' ">net8.0</LibraryTargetFrameworks>
<NetCoreOnlyTargetFrameworks>net10.0;net8.0</NetCoreOnlyTargetFrameworks>
<NetCoreOnlyTargetFrameworks Condition="
'$(IsLegacyVisualStudioVersion)' == 'true'
">net8.0</NetCoreOnlyTargetFrameworks>
</PropertyGroup>
```
While C# 14.0 isn't added in this PR, we will need to eliminate targets on
`netstandard2.0` and .NET Framework due to invalid warnings generated by Visual
Studio 2022 when using `string` > `ReadOnlySpan<char>` intrinsic conversions.
It is probably simpler just to add that logic now so we don't have to revisit
it again later. Note that it only applies to the IDE itself, not the command
line or CI.
##########
TestTargetFramework.props:
##########
@@ -29,21 +29,25 @@
<!--<TargetFramework>net472</TargetFramework>-->
<!--<TargetFramework>net48</TargetFramework>-->
<!--<TargetFramework>net8.0</TargetFramework>-->
- <TargetFramework>net9.0</TargetFramework>
+ <!-- Default for local development - conditional based on VS version
(VS2024+ = 18.0) -->
+ <TargetFramework Condition=" '$(VisualStudioVersion)' >= '18.0'
">net10.0</TargetFramework>
+ <TargetFramework Condition=" '$(VisualStudioVersion)' < '18.0' Or
'$(VisualStudioVersion)' == '' ">net8.0</TargetFramework>
<!-- Allow the build script to pass in the test frameworks to build for.
This overrides the above TargetFramework setting.
LUCENENET TODO: Due to a parsing bug, we cannot pass a string with a ;
to dotnet msbuild, so passing true as a workaround -->
<!-- Test Client to DLL target works as follows:
Test Client | Target Under Test
+ net10.0 | net10.0
net9.0 | net8.0
net8.0 | net8.0
net48 | net462
net472 | netstandard2.0
-->
- <TargetFrameworks Condition=" '$(TestFrameworks)' == 'true'
">net9.0;net8.0</TargetFrameworks>
+ <!-- CI builds - always build all test frameworks (CI installs required
SDKs) -->
+ <TargetFrameworks Condition=" '$(TestFrameworks)' == 'true'
">net10.0;net9.0;net8.0</TargetFrameworks>
<TargetFrameworks Condition=" '$(TestFrameworks)' == 'true' AND
$([MSBuild]::IsOsPlatform('Windows'))
">$(TargetFrameworks);net48;net472</TargetFrameworks>
Review Comment:
We will need to add an exclusion for VS 2022 here (this is for C# 14.0
support, which we can add later).
```xml
<TargetFrameworks Condition=" '$(TestFrameworks)' == 'true' AND
$([MSBuild]::IsOsPlatform('Windows')) And '$(IsLegacyVisualStudioVersion)' !=
'true' ">$(TargetFrameworks);net48;net472</TargetFrameworks>
```
##########
TestTargetFramework.props:
##########
@@ -29,21 +29,25 @@
<!--<TargetFramework>net472</TargetFramework>-->
<!--<TargetFramework>net48</TargetFramework>-->
<!--<TargetFramework>net8.0</TargetFramework>-->
- <TargetFramework>net9.0</TargetFramework>
+ <!-- Default for local development - conditional based on VS version
(VS2024+ = 18.0) -->
+ <TargetFramework Condition=" '$(VisualStudioVersion)' >= '18.0'
">net10.0</TargetFramework>
Review Comment:
The way this is currently specified, it will not default to `net10.0` on any
IDE that is not Visual Studio 2026 or higher, including Rider or Visual Studio
Code. It would be more sensible to downgrade automatically iif it is a legacy
VS version.
```xml
<TargetFrameworks>net10.0</TargetFrameworks>
<!-- For Visual Studio 2022, there is no support for net10.0, so we test on
net8.0 -->
<TargetFrameworks Condition=" '$(IsLegacyVisualStudioVersion)' == 'true'
">net8.0</TargetFrameworks>
```
See the comment in the root `Directory.Build.targets` for the definition of
`IsLegacyVisualStudioVersion`.
##########
src/dotnet/Lucene.Net.Tests.CodeAnalysis/Lucene.Net.Tests.CodeAnalysis.csproj:
##########
@@ -22,11 +22,11 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFramework>net9.0</TargetFramework>
+ <TargetFramework>net10.0</TargetFramework>
Review Comment:
We will need to add an exception for Visual Studio 2022 here or simply leave
this on `net9.0` or `net8.0`.
```xml
<TargetFrameworks>net10.0</TargetFrameworks>
<!-- For Visual Studio 2022, there is no support for net10.0, so we test on
net8.0 -->
<TargetFrameworks Condition=" '$(IsLegacyVisualStudioVersion)' == 'true'
">net8.0</TargetFrameworks>
```
See the comment in the root `Directory.Build.targets` for the definition of
`IsLegacyVisualStudioVersion`.
##########
src/dotnet/Lucene.Net.Tests.CodeAnalysis/Lucene.Net.Tests.CodeAnalysis.csproj:
##########
@@ -22,11 +22,11 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFramework>net9.0</TargetFramework>
+ <TargetFramework>net10.0</TargetFramework>
<RootNamespace>Lucene.Net.CodeAnalysis</RootNamespace>
<IsPublishable>false</IsPublishable>
- <IsPublishable Condition=" '$(TargetFramework)' == 'net9.0'
">true</IsPublishable>
+ <IsPublishable Condition=" '$(TargetFramework)' == 'net10.0'
">true</IsPublishable>
Review Comment:
Like the above, we will need this to be publishable on the same platform we
use for testing and don't publish any other platform. Note that publishing is
not currently executed in the IDE, but this is just in case it is attempted.
```xml
<IsPublishable Condition=" '$(TargetFramework)' == 'net10.0' And
'$(IsLegacyVisualStudioVersion)' != 'true' ">true</IsPublishable>
<IsPublishable Condition=" '$(TargetFramework)' == 'net8.0' And
'$(IsLegacyVisualStudioVersion)' == 'true' ">true</IsPublishable>
```
##########
src/Lucene.Net.Tests.Analysis.OpenNLP/Lucene.Net.Tests.Analysis.OpenNLP.csproj:
##########
@@ -26,7 +26,7 @@
<PropertyGroup>
<!-- Allow specific target framework to flow in from
TestTargetFrameworks.props -->
<!--suppress MsbuildTargetFrameworkTagInspection - even though this only
has one target right now, we need to use the plural version for the line below
-->
- <TargetFrameworks Condition=" '$(TargetFramework)' == ''
">net9.0;net8.0</TargetFrameworks>
+ <TargetFrameworks Condition=" '$(TargetFramework)' == ''
">net10.0;net9.0;net8.0</TargetFrameworks>
Review Comment:
We will need to conditionally exclude Visual Studio < 2026 here.
```xml
<TargetFrameworks Condition=" '$(TargetFramework)' == '' And
'$(IsLegacyVisualStudioVersion)' != 'true' ">net10.0</TargetFrameworks>
<TargetFrameworks Condition=" '$(TargetFramework)' == ''
">$(TargetFrameworks);net9.0;net8.0</TargetFrameworks>
<TargetFrameworks Condition=" $([MSBuild]::IsOsPlatform('Windows')) And
'$(IsLegacyVisualStudioVersion)' != 'true'
">$(TargetFrameworks);net48</TargetFrameworks>
```
See the comment in the root `Directory.Build.targets` for the definition of
`IsLegacyVisualStudioVersion`.
##########
src/Lucene.Net.Analysis.OpenNLP/Lucene.Net.Analysis.OpenNLP.csproj:
##########
@@ -32,8 +32,9 @@
<PropertyGroup>
<!-- Currently, IKVM doesn't officially support building NetFX on anything
but Windows, so we skip it for contributors who may be on various platforms.
We can remove the condition once that has been addressed. See:
https://github.com/ikvmnet/ikvm-maven/issues/49 -->
- <!--suppress MsbuildTargetFrameworkTagInspection - even though this only
has one target right now, we need to use the plural version for the line below
-->
- <TargetFrameworks>net8.0</TargetFrameworks>
+ <!-- net10.0 is conditional on VS version (VS2024+ = 18.0) -->
+ <TargetFrameworks Condition=" '$(VisualStudioVersion)' >= '18.0'
">net10.0;net8.0</TargetFrameworks>
Review Comment:
The way this is currently specified, it will not compile `net10.0` on any
IDE that is not Visual Studio 2026 or higher, including Rider or Visual Studio
Code. It would be more sensible to reject iif it is a legacy VS version.
```xml
<TargetFrameworks Condition=" '$(IsLegacyVisualStudioVersion)' != 'true'
">net10.0</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworks);net8.0</TargetFrameworks>
<TargetFrameworks Condition=" $([MSBuild]::IsOsPlatform('Windows')) And
'$(IsLegacyVisualStudioVersion)' != 'true'
">$(TargetFrameworks);net472</TargetFrameworks>
```
See the comment in the root `Directory.Build.targets` for the definition of
`IsLegacyVisualStudioVersion`.
##########
src/dotnet/Lucene.Net.ICU/Lucene.Net.ICU.csproj:
##########
@@ -30,7 +30,7 @@
<Import Project="$(SolutionDir).build/nuget.props" />
<PropertyGroup>
- <TargetFrameworks>net8.0;netstandard2.0;net462</TargetFrameworks>
+ <TargetFrameworks>net10.0;net8.0;netstandard2.0;net462</TargetFrameworks>
Review Comment:
This library is our replacement for what would have been
`Lucene.Net.Analysis.ICU`, except that it also contains some of the things from
`Lucene.Net.Analysis.Common` that require a `BreakIterator` and other support
from ICU to function. So, technically, it can use `$(LibraryTargetFrameworks)`
just like the other libraries.
##########
src/dotnet/tools/Lucene.Net.Tests.Cli/Lucene.Net.Tests.Cli.csproj:
##########
@@ -25,7 +25,7 @@
<PropertyGroup>
<!-- Allow specific target framework to flow in from
TestTargetFrameworks.props -->
- <TargetFrameworks Condition=" '$(TargetFramework)' == ''
">net9.0;net8.0</TargetFrameworks>
+ <TargetFrameworks Condition=" '$(TargetFramework)' == ''
">net10.0;net9.0;net8.0</TargetFrameworks>
Review Comment:
We will need to add an exception for Visual Studio 2022 here.
```xml
<TargetFrameworks>net10.0;net9.0;net8.0</TargetFrameworks>
<!-- For Visual Studio 2022, there is no support for net10.0, so we test on
net9.0 and net8.0 -->
<TargetFrameworks Condition=" '$(IsLegacyVisualStudioVersion)' == 'true'
">net9.0;net8.0</TargetFrameworks>
```
See the comment in the root `Directory.Build.targets` for the definition of
`IsLegacyVisualStudioVersion`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
