-
Notifications
You must be signed in to change notification settings - Fork 632
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
Upgrade tests and CLI to .NET 8, fix benchmark formatting issue #930
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build succeeds on Azure DevOps now, but there are some issues with the x86 tests not running. I think it is due to a duplicate installation of the .NET SDK. But before that is fixed, we should fix the crash detection to account for yet another way a build can fail so we can see the failure if something like this happens in the future. See my inline comments for details.
I setup a new repository with our Azure DevOps templates and discovered that there are some breaking changes. Breaking Change 1There is a breaking change introduced by Microsoft in the .NET 8 SDK, which appends the Git commit hash to the More info about it is here: https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/source-link. I have tested the solution and it seems to meet our needs. However, the first build after I changed it caused the NuGet packages to be built without any DLLs in them, but the second build succeeded. Not sure whether that was a fluke or a problem. We can put the fix in the default <PropertyGroup Label="Versioning">
<IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>
</PropertyGroup> Breaking Change 2The default for a new pipeline in Azure DevOps is to create a shallow clone. Previously, it would create a deep clone by default. This doesn't break anything on Lucene.Net, but all of the other repositories that we own use NerdBank.GitVersioning, and this is a problem for generating the correct version number. There is an issue about it here: dotnet/Nerdbank.GitVersioning#837 and the fix is here: https://github.com/dotnet/Nerdbank.GitVersioning/blob/main/doc/cloudbuild.md#azure-pipelines We would need to apply that on the "Build" job of all of our repositories. On Lucene.NET, it is already done this way for performance reasons. Breaking Change 3We do a solution-wide publish for the tests on Azure DevOps. This puts all of the DLL files into a single directory as they would be in the real world. So, doing it this way can detect issues with version conflicts between dependencies (in particular, transitive dependencies of libraries that we depend upon). The failure will happen in our test environment rather than waiting for a user to eventually depend on 2 of our packages with conflicting dependency versions. Unfortunately, Microsoft realized that solution-wide publish to a shared directory will cause "inconsistent builds" and made it fail if you provide the However, it is a bit more nuanced than that. In dotnet publish --output "$outputPath" --framework "$framework" --configuration "$configuration" --no-build --no-restore --verbosity normal /p:TestAllTargetFrameworks=true For solutions still supporting older versions of .NET Framework, we will need to continue publishing to a single directory to find conflicts. We should publish to separate directories for newer versions of .NET, so there will need to be some branching logic in those solutions. Since we use the same templates, we should just bake this logic in even if the current repo doesn't need it. I think we may be able to use the new simplified output path feature of .NET 8 mentioned here for .NET Framework 4.5.1 and higher. For .NET Framework 4.5 and lower, use: dotnet publish --framework "$framework" --configuration "$configuration" --no-build --no-restore --verbosity normal /p:"PublishDir=$outputPath" /p:TestAllTargetFrameworks=true I haven't gone into all of the nuance of this feature yet, though. We may consider folding our $testBinaries = Get-ChildItem -Path "$testBinaryRootDirectory" -File -Recurse | Where-Object {$_.FullName -match "$framework"} | Where-Object {$_.FullName -match "$fileRegexPattern"} | Select -ExpandProperty FullName So, we aren't tightly coupled to any particular directory structure. It just needs to be tested to confirm it can find test files with this new directory structure. It won't if the target framework is missing from the directory path, but based on their simple examples it isn't clear whether it will be there or not. Publish takes much longer going one project at a time, so we should continue publishing at the solution level, though. Our Other RepositoriesThis is a list of all of the repositories that use the same Azure DevOps (and command line build) templates that will need to be patched to account for this.
While we could just fix the issue in this PR and add 2 issues to each of the other repositories, I think it might be simpler just to submit a PR to each repository with the patches and a link back to this PR. |
.build/runbuild.ps1
Outdated
@@ -27,7 +27,7 @@ properties { | |||
[string]$testResultsDirectory = "$artifactsDirectory/TestResults" | |||
[string]$publishDirectory = "$artifactsDirectory/Publish" | |||
[string]$solutionFile = "$baseDirectory/Lucene.Net.sln" | |||
[string]$minimumSdkVersion = "8.0.202" | |||
[string]$minimumSdkVersion = "8.0.101" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am working on this now. Figured we would need to merge it before dealing with #935.
I ran into this issue: dotnet/runtime#88283. Still trying to come up with a solution. For whatever reason, just installing is not enough to get it to find the x86 host anymore.
…ion from Directory.Build.targets so it is with other version properties.
…'t a valid version)
…1 to 17.9.0 and NUnit3TestAdapter from 3.17.0 to 4.5.0
…ce they no longer function on .NET 8 with a default install.
…Apparently NUnit3TestAdapter 4.5.0 doesn't support net461.
… want users to be able to build with any .NET 8 SDK.
…or TestFrameworks parameter to reflect the new default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix for installing the right SDK based on target framework and platform turned out to be quite an ugly piece of code with lots of repeating and special cases. But it works. It took a lot of work because .NET 8 broke quite a few things, like support for .NET Framework 4.6.1 on NUnit.
I think it could be improved by creating a separate install-dotnet-sdk-for-target-framework.yml
template and putting logic in it to decide which SDKs to install based on target framework and architecture and to make it xplat. But we can probably wait until testing on ARM64 is possible in Azure DevOps before making the changes.
I don't imagine I changed anything that could cause a local build to fail, but as long as this configuration still works on ARM64, it is good to merge pending your review of my changes.
This looks great, thanks for figuring that out! |
.NET 8 upgrade for test and CLI projects.
Partial #928
Description
FEATURE_SERIALIZABLE
andFEATURE_SERIALIZABLE_EXCEPTIONS
for net7.0+ where BinaryFormatter is deprecated (and throws an exception in .NET 8)NOTE: net6.0 is still the latest target for the libraries at this time. This just ensures we're testing with .NET 8, as well as providing a .NET 8 CLI.