Re: [PR] Upgrade tests and CLI to .NET 8, fix benchmark formatting issue [lucenenet]

["NightOwl888 (via GitHub)" <gi...@apache.org>]

NightOwl888 commented on PR #930:
URL: https://github.com/apache/lucenenet/pull/930#issuecomment-2031124211

   I setup a new repository with our Azure DevOps templates and discovered that there are some breaking changes.
   
   ### Breaking Change 1
   
   There is a breaking change introduced by Microsoft in the .NET 8 SDK, which appends the Git commit hash to the `AssemblyInformationalVersion`: https://github.com/dotnet/sdk/issues/34568. We already add a shortened Git commit hash, so this will add it twice. The fix should definitely be in this PR, since this is where we introduce building with the .NET 8 SDK.
   
   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 `Directory.Build.targets` file of the solution (in each repository):
   
   ```xml
     <PropertyGroup Label="Versioning">
       <GenerateAssemblyVersionInfo>false</GenerateAssemblyVersionInfo>
     </PropertyGroup>
   ```
   
   ### Breaking Change 2
   
   The 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: https://github.com/dotnet/Nerdbank.GitVersioning/issues/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 3
   
   We 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 `--output` parameter when publishing on a solution. Here is where I found a solution that works: https://stackoverflow.com/a/75556105 on .NET Framework 4.5 and lower.
   
   However, it is a bit more nuanced than that. [`net451` fixed the automation of assembly binding redirects'](https://learn.microsoft.com/en-us/dotnet/framework/configure-apps/how-to-enable-and-disable-automatic-binding-redirection#disable-automatic-binding-redirects-in-desktop-apps). 
   
   In `build-pack-and-publish-libraries.yml`, we need to change:
   
   ```powershell
   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](https://devblogs.microsoft.com/dotnet/announcing-dotnet-8-preview-3/#simplified-output-path) feature of .NET 8 mentioned [here](https://github.com/dotnet/msbuild/issues/3497#issuecomment-1560451928) for .NET Framework 4.5.1 and higher.
   
   For .NET Framework 4.5 and lower, use:
   
   ```powershell
   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 `_artifacts` directory into the new `.artifacts` directory of this feature. And we generally don't want it enabled except in our CI and command-line build scripts. Our tests search all subdirectories for the test DLL name pattern below directories with the name of the target framework:
   
   ```powershell
   $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 Repositories
   
   This 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.
   
   - [J2N](https://github.com/NightOwl888/J2N)
   - [ICU4N](https://github.com/NightOwl888/ICU4N)
   - [RandomizedTesting](https://github.com/NightOwl888/RandomizedTesting)
   - [Spatial4n](https://github.com/Spatial4n/Spatial4n)
   - [Morfologik.Stemming](https://github.com/NightOwl888/Morfologik.Stemming)
   - [NetFx.Polyfills](https://github.com/NightOwl888/NetFx.Polyfills) (fixes already applied)
   
   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.


-- 
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: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org