Modernize to .NET 10 / C# 14, fix upstream bugs, Android 16KB page alignment, one-click NuGet CI#1
Conversation
…amespaces and latest patterns Agent-Logs-Url: https://github.com/winnerspiros/veldrid-spirv/sessions/cdba2021-18f0-4917-82ff-e741483fa17f Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
…int fix, exception line numbers, Android 16KB page size, modernize CI workflow to .NET 10 with one-click NuGet publish Agent-Logs-Url: https://github.com/winnerspiros/veldrid-spirv/sessions/cdba2021-18f0-4917-82ff-e741483fa17f Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
…nSerializer, consolidate constructors, switch expressions, updated README with changelog from ppy fork Agent-Logs-Url: https://github.com/winnerspiros/veldrid-spirv/sessions/cdba2021-18f0-4917-82ff-e741483fa17f Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
…t null, string interpolation, expression bodies, LINQ simplification Agent-Logs-Url: https://github.com/winnerspiros/veldrid-spirv/sessions/cdba2021-18f0-4917-82ff-e741483fa17f Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Modernization and maintenance update that upgrades the repo to .NET 10/C# latest, pulls in upstream bug fixes for shader compilation/cross-compilation, adds Android 16KB page-size linker alignment, and streamlines CI/NuGet publishing.
Changes:
- Upgrade projects to
net10.0with nullable + implicit usings; refactor to modern C# patterns across the codebase. - Fix/port upstream interop and diagnostics issues (debug flag propagation, specialization constant marshaling, better error metadata).
- Update native/CI tooling (Android 16KB page alignment, linux-arm64 test matrix, one-click NuGet publish input).
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libveldrid-spirv/libveldrid-spirv.cpp | Adds missing <cstdint> include for GCC 10+ compatibility. |
| src/Veldrid.SPIRV/VertexFragmentCompilationInfo.cs | Converts to file-scoped namespace; makes reflection nullable in result type. |
| src/Veldrid.SPIRV/VeldridSpirvNative.cs | Refactors to file-scoped namespace; keeps iOS resolver logic. |
| src/Veldrid.SPIRV/Veldrid.SPIRV.csproj | Targets net10.0; adds packed README for NuGet. |
| src/Veldrid.SPIRV/Util.cs | Modernizes nullability and file-scoped namespace. |
| src/Veldrid.SPIRV/SpirvReflection.cs | Refactors to file-scoped namespace and modern formatting. |
| src/Veldrid.SPIRV/SpirvCompilationResult.cs | Refactors to file-scoped namespace and modern formatting. |
| src/Veldrid.SPIRV/SpirvCompilationException.cs | Refactors to file-scoped namespace and modern formatting. |
| src/Veldrid.SPIRV/SpirvCompilation.cs | Major modernization; fixes specialization constant interop; adds richer error metadata. |
| src/Veldrid.SPIRV/ShadercShaderKind.cs | Refactors to file-scoped namespace and modern formatting. |
| src/Veldrid.SPIRV/ResourceFactoryExtensions.cs | Fixes debug flag propagation; refactors to modern C# constructs. |
| src/Veldrid.SPIRV/ReflectionInfo.cs | Refactors interop structs to file-scoped namespace. |
| src/Veldrid.SPIRV/NativeSpecializationConstant.cs | Refactors interop struct to file-scoped namespace. |
| src/Veldrid.SPIRV/NativeMacroDefinition.cs | Adds validation + refactor; introduces MaxBufferSize constant. |
| src/Veldrid.SPIRV/MacroDefinition.cs | Enables nullable value; modernizes constructors/properties. |
| src/Veldrid.SPIRV/InteropArray.cs | Hardens bounds checks; refactors layout to file-scoped namespace. |
| src/Veldrid.SPIRV/GlslCompileOptions.cs | Enables nullable usage patterns; uses collection expressions for defaults. |
| src/Veldrid.SPIRV/GlslCompileInfo.cs | Refactors interop struct to file-scoped namespace. |
| src/Veldrid.SPIRV/CrossCompileTarget.cs | Refactors enum to file-scoped namespace with docs cleanup. |
| src/Veldrid.SPIRV/CrossCompileOptions.cs | Consolidates constructors; modernizes defaults and initialization. |
| src/Veldrid.SPIRV/CrossCompileInfo.cs | Refactors interop struct to file-scoped namespace. |
| src/Veldrid.SPIRV/ComputeCompilationResult.cs | Refactors to file-scoped namespace and modern formatting. |
| src/Veldrid.SPIRV/CompilationResult.cs | Modernizes bounds checks; refactors interop struct formatting. |
| src/Veldrid.SPIRV/Bool32.cs | Refactors to file-scoped namespace; minor modernization. |
| src/Veldrid.SPIRV.VariantCompiler/Veldrid.SPIRV.VariantCompiler.csproj | Targets net10.0; updates CLI utils dependency. |
| src/Veldrid.SPIRV.VariantCompiler/Program.cs | Modernizes CLI program and JSON load flow. |
| src/Veldrid.SPIRV.VariantCompiler/CompilationSet.cs | Refactors variant compiler; improves error aggregation; extracts JSON writing helper. |
| src/Veldrid.SPIRV.Tests/Veldrid.SPIRV.Tests.csproj | Targets net10.0; migrates to xunit.v3 and modern test SDK. |
| src/Veldrid.SPIRV.Tests/TestUtil.cs | Refactors to file-scoped namespace and modern formatting. |
| src/Veldrid.SPIRV.Tests/ReflectionTests.cs | Updates tests for nullable reflection and modern C# syntax. |
| src/Veldrid.SPIRV.Tests/Program.cs | Removes deprecated custom xUnit console runner. |
| src/Veldrid.SPIRV.Tests/CompilationTests.cs | Updates tests to modern C# syntax and file-scoped namespace. |
| src/Veldrid.SPIRV.BuildTools/Veldrid.SPIRV.BuildTools.csproj | Targets net10.0; updates published tool path. |
| README.md | Updates documentation, changelog, CI instructions, and fixes sample code. |
| Directory.Build.props | Enables nullable/implicit usings/lang latest; switches to license expression; bumps Nerdbank.GV. |
| CMakeLists.txt | Adds Android 16KB page-size linker flag. |
| .github/workflows/build.yml | Updates CI for .NET 10, linux-arm64 tests, and manual NuGet publish input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <param name="vertexElements">/// An array containing a description of each vertex element that is used by | ||
| /// the compiled shader set.</param> | ||
| /// <param name="resourceLayouts">An array containing a description of each set of resources used by the |
There was a problem hiding this comment.
The XML doc for vertexElements has an extra /// inside the <param> text, which will show up in generated docs and is likely unintended. Remove the stray /// so the parameter description renders correctly.
| string reflectionPath = Path.Combine(_outputPath, $"{variant.Name}_ReflectionInfo.json"); | ||
| WriteReflectionJson(reflectionPath, result.Reflection); | ||
| generatedFiles.Add(reflectionPath); |
There was a problem hiding this comment.
CompileCompute writes *_ReflectionInfo.json inside the per-target loop, so the same file is overwritten for each target and the returned generatedFiles can contain duplicate reflection paths. Consider writing reflection once (e.g., on first successful target, similar to CompileVertexFragment) and only adding the path once.
| <ItemGroup> | ||
| <PackageReference Include="Veldrid.SPIRV" Version="1.0.14" /> | ||
| <PackageReference Include="McMaster.Extensions.CommandLineUtils" Version="4.0.1" /> | ||
| <PackageReference Include="McMaster.Extensions.CommandLineUtils" Version="5.1.0" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
This project is part of src/Veldrid.SPIRV.sln, but it references the Veldrid.SPIRV NuGet package instead of the in-repo Veldrid.SPIRV project. That can lead to building/running the VariantCompiler against a different version than the rest of the solution. Consider switching this to a ProjectReference (or aligning the package reference to the fork’s package ID/version) to avoid version skew.
| <Content Include="TestShaders/*" CopyToOutputDirectory="PreserveNewest" /> | ||
| <Content Include="$(RepositoryRootDirectory)/build/$(Configuration)/win-x64/libveldrid-spirv.dll" Condition="Exists('$(RepositoryRootDirectory)/build/$(Configuration)/win-x64/libveldrid-spirv.dll')" CopyToOutputDirectory="PreserveNewest" /> | ||
| <Content Include="$(RepositoryRootDirectory)/build/$(Configuration)/osx/libveldrid-spirv.dylib" Condition="Exists('$(RepositoryRootDirectory)/build/$(Configuration)/osx/libveldrid-spirv.dylib')" CopyToOutputDirectory="PreserveNewest" /> | ||
| <Content Include="$(RepositoryRootDirectory)/build/$(Configuration)/linux-x64/libveldrid-spirv.so" Condition="Exists('$(RepositoryRootDirectory)/build/$(Configuration)/linux-x64/libveldrid-spirv.so')" CopyToOutputDirectory="PreserveNewest" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
The workflow now runs tests on linux-arm64, but the test project only copies the linux-x64 native library into the output. On ARM runners this likely results in DllNotFoundException at runtime. Add a conditional content item for build/$(Configuration)/linux-arm64/libveldrid-spirv.so (or otherwise ensure the ARM64 .so is placed on the test output path).
| create: # when tags are created | ||
| push: | ||
| branches: [ master ] |
There was a problem hiding this comment.
on: create triggers for both branch and tag creation, but the comment suggests it is intended only for tags. If the goal is to run CI only for tags, consider removing create and using push: tags: (or add job-level if: guards) to avoid running the full matrix on branch creation events.
| create: # when tags are created | |
| push: | |
| branches: [ master ] | |
| push: | |
| branches: [ master ] | |
| tags: [ '*' ] |
Full modernization pass: upgrade from .NET 5/8 to .NET 10 with latest C#, incorporate all open bug fixes from upstream
veldrid/veldrid-spirv, and make NuGet publishing a single workflow button click..NET & Language
net10.0,LangVersion=latest(C# 14),Nullable=enable,ImplicitUsings=enableis null/is not null, switch expressions, source-generated[GeneratedRegex], expression-bodied members, primary constructors, string interpolationDotNetCliToolReference,System.Runtime.CompilerServices.Unsafe, test runnerProgram.csPackageLicenseUrl→PackageLicenseExpressionUpstream Bug Fixes
Debugflag now propagated to cross-compiled shader descriptions inResourceFactoryExtensions#include <cstdint>added for GCC 10+ native build compatSpirvCompilationException.Datanow includesLineNumberandShaderStageCompileComputeinterop bug: Was marshalingSpecializationConstant(managed) instead ofNativeSpecializationConstantto native code — potential memory corruptionAndroid 16KB Page Size
Required for Android 16 (API 36+). Without this,
libveldrid-spirv.sofails the page size check on devices enforcing 16KB pages.Package Updates
CI/CD
dotnet run -p→dotnet testlinux-arm64to test matrixworkflow_dispatchnow has apublish_nugetboolean input — one-click NuGet publish from the Actions tab--skip-duplicateondotnet nuget pushCode Quality
ArgumentNullException.ThrowIfNull, removed unsafe!null-forgiving operators, added explicit null guards inVariantCompilerJsonSerializersetup intoWriteReflectionJsonhelperCrossCompileOptionsconstructors into 2 with delegationwriteReflectionFileflag logic (renamed toisFirstTarget)