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.

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

Target .NET 8 #928

Closed
1 task done
Zyklop opened this issue Mar 11, 2024 · 16 comments · Fixed by #982
Closed
1 task done

Target .NET 8 #928

Zyklop opened this issue Mar 11, 2024 · 16 comments · Fixed by #982
Assignees

Comments

@Zyklop
Copy link

Zyklop commented Mar 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Officially the latest release only supports .Net 6.0. With 6.0 going out of support this year it would be good to support the newer versions. (.Net 8.0) Are there any expected issues with the upgrade?

Describe the solution you'd like

Added compatibility for .Net 8

Additional context

No response

@rclabo
Copy link
Contributor

rclabo commented Mar 11, 2024

The project supports and works great with the latest .NET versions including .NET 7 and .NET 8. When you say that "Officially the latest release only supports .Net 6.0" what are you referring to?

@Zyklop
Copy link
Author

Zyklop commented Mar 11, 2024

Thank you for the quick reply.
I just meant to extend the targetframework
net6.0;netstandard2.1;netstandard2.0;net462 and add net7.0;net8.0

@paulirwin
Copy link
Contributor

We should not add net7.0 at this time as it will be out of support in a couple months. However, net8.0 should be supported. We should be able to do this fairly easily as it's not a big jump from 6 to 8. @NightOwl888 has already updated the CI to build/test with 8 anyways.

Of course, for posterity, there's nothing stopping you from using Lucene.net 4.8 beta in .NET 7+ today, even though it doesn't target it.

@paulirwin paulirwin changed the title Any plans for supporting .Net7.0 or .Net 8.0? Target .NET 8 Mar 11, 2024
@paulirwin paulirwin self-assigned this Mar 11, 2024
@NightOwl888
Copy link
Contributor

NightOwl888 commented Mar 11, 2024

What is going out of support is the .NET 6 runtime. The target framework net6.0 won't be going out of support and (barring any major screwup from Microsoft) will support all future runtimes of .NET Core. When a project loads a class library that targets an earlier target framework, it loads whatever runtime is being used by the executable loading the class library. A target framework for a class library is a set of APIs, not a runtime.

You can see for yourself if you try running a project with a net8.0 executable and only install the .NET 8 runtime on the machine that loads a net6.0 targeted DLL. It still works because the .NET 6 runtime doesn't need to be loaded.

I suspect we probably wouldn't be having this conversation if we had just stuck with netstandard2.1, which hasn't ever had a runtime and never added support for net6.0. But there were performance advantages with targeting net6.0 so we added support.

We do need to add tests for .NET 8.0, but barring any additional performance improvements that are discovered in APIs that only exist in .NET 8.0, there really is no compelling reason to increment our target framework until after the runtime is long gone. .NET 6 is the minimum version of the .NET Core runtime that is supported by net6.0. I have already analyzed the source for the primary performance benefit (hardware intrinsics) and that has not changed at all in .NET 7.0 or 8.0.

@NightOwl888
Copy link
Contributor

@paulirwin

@NightOwl888 has already updated the CI to build/test with 8 anyways.

Actually, not. I recently re-partitioned my machine because I didn't have the disk space to install the .NET 8 SDK, but that has been addressed so we can move forward.

@paulirwin
Copy link
Contributor

Ah I was thinking of b1476ae which I see now was for the Sonar pipeline.

I think we can keep this issue open for future .NET 8 targeting, and agreed with all the points above.

@paulirwin paulirwin removed their assignment Mar 11, 2024
@NightOwl888
Copy link
Contributor

NightOwl888 commented Mar 11, 2024

As for adding a net8.0 target framework for distribution, we should wait on this until we can phase out the net6.0 target in November, 2024. We have fairly large resource files that are embedded in some of the assemblies, so each target framework makes our distribution size several tens of MB larger.

So, there is a cost-benefit tradeoff here:

  1. If we put in the time to move the resource files into satellite assemblies, we can add more target frameworks without it making our distribution size grow proportionally per target framework.
  2. If we wait for runtimes to go out of support before changing target frameworks, we can simply swap one target framework for another and always cap our distribution at 4 or 5 target frameworks (or even 3).
  3. If we do neither and add target frameworks earlier rather than later, there is a risk we will eventually go over the 1GB release limit and have to explicitly notify infra when we do a release. Small chance of this, though - we are currently at ~170MB, but that will change when adding a net8.0 target in lucene-cli.

lucene-cli Roll Forward Support

There is also the matter of the one executable that we distribute - lucene-cli. We need to add .NET 8 support ASAP. We multi-target all of the latest supported runtimes and should continue to do so as new runtimes are released and phased out. This makes the distribution size larger than any other .nupkg file we distribute.

2-3 years after a Lucene.NET release, Microsoft will drop support for all runtimes in that release. For now, our highest supported runtime uses the Major roll-forward policy. Currently we have no documentation on how to use this and I am not sure whether it will work with versions higher than .NET 7.0 with our current setting and set of target frameworks.

Documentation won't be required if Microsoft finishes the UX to make it automatically default to the latest runtime, though (dotnet/sdk#30336). So, if they release that before our release we should be fine. But if not, we should document this so years after a given release users will have the ability to use the original tools that shipped with it on the latest .NET runtime.

@NightOwl888
Copy link
Contributor

NightOwl888 commented Mar 12, 2024

Documentation won't be required if Microsoft finishes the UX to make it automatically default to the latest runtime, though (dotnet/sdk#30336). So, if they release that before our release we should be fine. But if not, we should document this so years after a given release users will have the ability to use the original tools that shipped with it on the latest .NET runtime.

Scratch that, it doesn't sound like this is a complete replacement for documentation. Their plan seems to primarily to keep the upgrade explicit instead of implicit, but fix it so the installation will fail if the proper runtime is missing with the instructions on how to roll forward.

We should also make it more clear in the README and on the website that we currently support the latest .NET core runtime, even future runtimes.

So, there are 4 tasks to take away from this. The first 3 will need to be done in this order:

  1. Experimentally work out how to document the lucene-cli roll forward experience to the latest .NET runtime. The ideal place to put this would be on the index page for lucene-cli. This will require a machine without .NET 6 or .NET 7 SDK or runtime installed. We need a lack of explicit support for .NET 8 in order to work this out. Note this will require using the --add-source argument of dotnet tool install to work with a local directory containing the build of the lucene-cli .nupkg file.
  2. Add tests for net8.0. At this point, we might just consider replacing net7.0 tests with net8.0 tests in CI rather than adding additional time to a build/test cycle.
  3. Add a target for net8.0 to lucene-cli. At this point, we might just consider replacing the net7.0 target with a net8.0 target.
  4. Update the README and website to indicate that we support all versions of .NET Core (except 1.x), including all future versions. Specifically, the line that reads Integrates well with .NET 6.0, .NET 5.0 and .NET Core 2+ should be more inclusive of future .NET Core runtimes. We should also change both places to read Supported Target Frameworks instead of Supported Frameworks and add a comment below that header to make it more clear we are not referring to the runtime and we support all higher runtimes that can depend on class assemblies of each of these target frameworks.

@eladmarg
Copy link
Contributor

on my machine i did see performance improvements by targeting net8.
actually for my projects i did manual migration to net8.

if someone is using net6, the upgrade path to 8 is very easy.

@NightOwl888
Copy link
Contributor

@eladmarg - Do you mean you compiled Lucene.NET with a net8.0 target and saw an improvement in performance when your net8.0 targeted application took a dependency on Lucene.NET with a net8.0 target vs a net6.0 target? If you instead meant you upgraded your project to net8.0 and used the net6.0 target of Lucene.NET, then a performance improvement is to be expected by using the newer runtime.

@eladmarg
Copy link
Contributor

yes, i replaced in the solution net6 -> net8, did some adjustments to make this compile, and see the improvement.

of course as @rclabo said, its working on net8 but performs faster when compiled as net8.

@rclabo
Copy link
Contributor

rclabo commented Mar 12, 2024

yes, i replaced in the solution net6 -> net8, did some adjustments to make this compile, and see the improvement.

I'm scratching my head at this. That could be true, but why would it be true? I mean, net8 is a faster environment but you'd get that speed improvement if your app is targeting net8 even if the LuceneNET library is not specifically targeting it, wouldn't you? (shrug)

@paulirwin
Copy link
Contributor

Regarding replacing net7.0 tests/CLI target with net8.0, I think we can and should go ahead and do this. .NET 7 will be out of support in two months, while .NET 8 will be supported for a few more years. And unlike the unsupported .NET 5 target for testing .NET Standard support, .NET 7 provides (AFAICT) no value beyond ensuring it works on .NET 7, which again should be mostly irrelevant in a few short months. I'll take this part on.

@NightOwl888
Copy link
Contributor

yes, i replaced in the solution net6 -> net8, did some adjustments to make this compile, and see the improvement.

I'm scratching my head at this. That could be true, but why would it be true? I mean, net8 is a faster environment but you'd get that speed improvement if your app is targeting net8 even if the LuceneNET library is not specifically targeting it, wouldn't you? (shrug)

I suspected this would be the case. I think this has to do with the fact that the BCL is spread across a lot of assemblies. If you depend on an assembly that targets net6.0, that assembly cannot depend on an assembly that targets net8.0 So if you load the former, you lose whatever performance advantages that were added to the latter.

For example, the Lucene.Net assembly is the main one that has performance improvements that call into hardware intrinsics. But if we didn't target net6.0 with the Lucene.Net.Analysis.Common assembly and the consuming application targeted net6.0, it would load the netstandard2.1 copy of Lucene.Net.Analysis.Common. Since that depends on Lucene.Net, you would also get the netstandard2.1 copy of Lucene.Net and it would skip the hardware intrinsics support and other improvements in Lucene.Net even though it has a net6.0 target. This is why we must keep the same targets across all submodules or we will end up with implicit downgrades.

So, what is probably happening with the net6.0 target under net8.0 is we are getting those implicit downgrades of the BCL assemblies that we depend upon.

@rclabo
Copy link
Contributor

rclabo commented Mar 12, 2024

@NightOwl888 Thanks for explaining this. That's a bit eye-opening for me.

@rclabo
Copy link
Contributor

rclabo commented Mar 12, 2024

Regarding replacing net7.0 tests/CLI target with net8.0, I think we can and should go ahead and do this. .NET 7 will be out of support in two months, while .NET 8 will be supported for a few more years. And unlike the unsupported .NET 5 target for testing .NET Standard support, .NET 7 provides (AFAICT) no value beyond ensuring it works on .NET 7, which again should be mostly irrelevant in a few short months. I'll take this part on.

I totally agree with this approach.

@NightOwl888 NightOwl888 added this to the 4.8.0-beta00017 milestone Oct 21, 2024
@paulirwin paulirwin self-assigned this Oct 21, 2024
paulirwin added a commit to paulirwin/lucene.net that referenced this issue Oct 21, 2024
paulirwin added a commit that referenced this issue Oct 22, 2024
* Target .NET 8, #928

* Lucene.Net.Store.NativeFSLockFactory: Added FEATURE_SUPPORTEDOSPLATFORMATTRIBUTE so we don't have an #ifdef hard coded to a specific target framework. This fixes the CA1416 warning after targeting net8.0, and will make future targeting simpler.

* Lucene.Net.Store.NativeFSLockFactory: Removed `using System.Runtime.Versioning` and sorted using directives

* Remove unused method

---------

Co-authored-by: Shad Storhaug <shad@shadstorhaug.com>
paulirwin added a commit to paulirwin/lucene.net that referenced this issue Oct 23, 2024
paulirwin added a commit to paulirwin/lucene.net that referenced this issue Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants