Skip to content

Conversation

@rory-cd
Copy link

@rory-cd rory-cd commented Dec 14, 2025

Description

Background and motivation

The SplashKit NuGet package currently provides C# bindings to SplashKit users (translated from C++ using SplashKit Translator). These bindings allow users to call upon SplashKit functions using C#. However, users must install both SplashKit Manager (SKM) and the SplashKit NuGet package to create SplashKit projects in C#.

The purpose of this PR is to add SplashKit native libraries (for Windows 64-bit and MacOS) to the NuGet package, allowing Windows and Mac users to create and run SplashKit projects in C# without the need for SKM. This would simplify installation and updates, improving the on-boarding experience for new users.

The need for a test suite

Given the scope of the update, extensive testing is necessary. A consistent and convenient test structure will be important to ensure thorough and timely testing is conducted. Therefore, a suite of C# test programs has been added to tools/scripts/test/. This directory originally contained a simple HelloWorld test, and is now superseded. The new tests are translations of a selection of the tests located at coresdk/src/test. Like the current tests, they are launched via a test runner (in coresdk/src/test/Main).

Note: While adding a test suite falls outside of the original scope of the task, the tests were judged to be integral to this PR, so were left in.

Existing contributions

Past team members had made some progress towards this goal:

  • Icon and description added
  • download-libraries.sh script, used to download the latest stable libraries
  • Commented suggestions for how to incorporate the libraries

Updates

  • Added native library references to build/packaging instructions in SplashKitSDK.csproj
  • Added test suite (C# solution) to tools/scripts/test
    • 8 tests translated from coresdk/src/test
    • Test runner in tools/scripts/test/Main used to launch tests
  • .gitignore updated to ignore downloaded libraries and intermediate build artifacts
  • Removed superseded tools/scripts/test/test.csproj
  • Removed SplashKit.cs symbolic link from NuGet folder, in favor of a compilation link in .csproj. Symbolic links are problematic in MSYS2 - see more here.
  • Removed commented suggestions from download-libraries.sh: These suggestions were implemented with adjustments in SplashKitSDK.csproj.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

All tests from included test suite successfully run on:

Windows 10 (MSYS2) Windows 10 (WSL) Windows 11 (MSYS2) Windows 11 (WSL) MacOS (Intel) MacOS (Apple Silicon)
.NET 6 - - - -
.NET 7 - - - -
.NET 8 - - - -
.NET 9 - - - -

Note: The included graphics test produces inconsistent results on Windows MSYS2. However, this error also occurs with the current (released) NuGet package, and even occurs in the original C++ integration test. Fixing this error falls outside the scope of this PR.

Testing guide for reviewers

A guide to SplashKit NuGet (including test instructions) has been written for the documentation website (PR#158). For now, the guide is best viewed via the Netlify deploy preview.

At minimum, when testing, follow the guide to:

  • Build the package
  • Use the provided test suite on the newly built package, with all four versions of .NET (complete a column in the table above)

For a test to be a considered a success (tick mark), every test must output the expected result. If the expected result is unclear or needs to be verified, the corresponding test from the standard SplashKit test suite in coresdk/src/test can be built and run to cross-reference.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from ... on the Pull Request

Allows C# users to utilise SplashKit without the installer, by
including the SplashKit native libraries within the NuGet package.
This solution mimicks the existing SplashKit tests, using a launcher
program (Main) to run focused test programs. Tests placed in the
solution will be used to check functionality of the SplashKit NuGet
package.
Adds a global settings class with a field referencing the existing
integration test resources. This allows for sharing of resources
with existing tests, reducing repo bloat.
Adds a selection of tests from the core integration tests to the
NuGet test collection. Tests have been translated from C++ to C#
with minimal other changes.
Removes the symbolic link to the C# bindings (SplashKit.cs) in favor
of a compilation link in .csproj. Symbolic links are not respected
by MSYS2, and therefore the NuGet package build was failing.
Using the MSBuild compile item improves cross-platform compatibility.
@rory-cd rory-cd marked this pull request as ready for review December 17, 2025 10:30
@222448082Ashen
Copy link

1. Code Quality & Structure

  • Well-organized test suite with clear separation of concerns
  • Clean C# code following .NET conventions
  • Proper use of Directory.Build.props for centralized configuration
  • Good resource sharing via GlobalSettings.cs

2. Functionality

  • Achieves stated goal of bundling native libraries into NuGet package
  • Removes SKM dependency for Windows/Mac users
  • Test runner provides excellent UX with framework version detection

3. Testing Approach

  • Comprehensive test coverage across 8 functional areas (Audio, Graphics, Input, Networking, Windows, etc.)
  • Multi-framework testing (.NET 6-9)
  • Tests translated from existing core test suite ensuring consistency
  • Clear testing matrix documented in PR description

4. Documentation

Areas for Improvement

1. Configuration & Build Files

Issue: SplashKitSDK.csproj has duplicate MacOS library entries

<!-- Line 52-58: osx-x64 -->
<Content Include=".\Libraries\macos\libSplashKit.dylib">
  <PackagePath>runtimes/osx-x64/native</PackagePath>
<!-- Line 62-68: osx-arm64 -->
<Content Include=".\Libraries\macos\libSplashKit.dylib">
  <PackagePath>runtimes/osx-arm64/native</PackagePath>

Recommendation: Verify if separate ARM64 and x64 libraries are needed. If libSplashKit.dylib is a universal binary, only one entry is needed. If separate binaries exist, update the download script and paths accordingly.

2. Backward Compatibility

Issue: Removing SplashKit.cs symbolic link may affect existing build scripts
Recommendation: Document this breaking change in release notes and migration guide

3. Linux Support

Issue: PR only adds Windows/Mac libraries
Recommendation: Add tracking issue for Linux native library support or document why it's excluded

4. Test Coverage

Observation: Graphics test has known MSYS2 issues (mentioned in PR)
Recommendation:

  • Add skip condition or expected failure documentation for MSYS2
  • Create issue to track graphics binding bug
  • Consider marking test as [ExpectedFailure] on Windows MSYS2

5. Dependencies

Issue: .gitignore excludes Libraries/ folder but it's required for build
Recommendation: Add README in Libraries/ folder explaining how to obtain libraries via download-libraries.sh

6. Security & Performance

Observation: Native libraries downloaded from external repo
Recommendation:

  • Add checksum verification to download-libraries.sh
  • Document library versioning strategy

Reviewer Checklist (Per Thoth Tech Guidelines)

[x] Code Quality - Aligns with C# and .NET standards
[x] Functionality - Achieves stated purpose
[x] Testing - Comprehensive test suite included
[x] Documentation - Well documented with companion PR
[x] Code Readability - Clean, well-structured code
[x] Maintainability - Modular design with shared settings
[] Edge Cases - MSYS2 graphics issue needs tracking
[x] Backward Compatibility - Changes documented
[x] Performance - No concerns identified
[x] Security - Consider checksum verification
[] Dependencies - Native library management needs documentation

Action Items

For Author:

  1. Clarify MacOS library architecture requirements (x64 vs ARM64 vs universal)
  2. Add checksum verification to download-libraries.sh
  3. Create README in Libraries/ folder with setup instructions
  4. Create tracking issue for Linux support
  5. Create tracking issue for MSYS2 graphics bug
  6. Add migration notes for symbolic link removal

For Reviewers:

  • Test on actual Windows and Mac environments with all .NET versions
  • Verify library download process
  • Test package installation from built NuGet

@rory-cd
Copy link
Author

rory-cd commented Jan 2, 2026

Thanks for the review @222448082Ashen. I have addressed the recommendations below. Could you please state the environment(s) you ran the tests on and the outcome (success/failure)?

1. Configuration & Build Files

Recommendation: Verify if separate ARM64 and x64 libraries are needed. If libSplashKit.dylib is a universal binary, only one entry is needed. If separate binaries exist, update the download script and paths accordingly.

This is a universal macOS-native library. It must be included in the runtimes/ path in the NuGet package, and NuGet will always select assets from the runtimes/{rid}/native/ directory. So while it initially seems unnecessary, the library must be duplicated across separate entries because that's how NuGet expects it. For more information please read more here - Example 3 show this design.

2. Backward Compatibility

Recommendation: Document this breaking change in release notes and migration guide

The removed symlink was only used internally during the package build to reference generated bindings. MSBuild implicitly includes any .cs files inside the project directory in its compilation, so the symlink avoided duplication. Since this is not public-facing, inline documentation has been added to SplashKitSDK.csproj.

3. Linux Support

Recommendation: Add tracking issue for Linux native library support or document why it's excluded

Yes, SKM doesn't include pre-compiled libraries for Linux. It instead uses a script to install dependencies and builds based on the detected distro. Including documentation on this is a good point - explanations have been added to download-libraries.sh and the documentation PR#158.

4. Test Coverage

Recommendation:

  • Add skip condition or expected failure documentation for MSYS2
  • Create issue to track graphics binding bug
  • Consider marking test as [ExpectedFailure] on Windows MSYS2

The expected failure is now noted in the documentation PR#158. A new ticket has been created to investigate this issue.

5. Dependencies

Recommendation: Add README in Libraries/ folder explaining how to obtain libraries via download-libraries.sh

The Libraries/ directory is generated by download-libraries.sh and is not tracked by the repo. Since it's a generated directory, it was added to the .gitignore to avoid accidental inclusion in future commits. The accompanying documentation covers how to run the script to retrieve the libraries.

6. Security & Performance

Recommendation:

  • Add checksum verification to download-libraries.sh

I agree that checksum verification would be an improvement. The deploy process doesn't currently generate a checksum, so the deploy script (tools/scripts/deploy.sh) would need to be updated to provide this function. Since this PR is already broad, I will add checksum generation/verification as part of a new PR to keep things scoped.

  • Document library versioning strategy

The targeted library versions are simply the "latest" - this is noted in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants