-
Notifications
You must be signed in to change notification settings - Fork 40
Add SplashKit native libraries to NuGet package, including test suite #149
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
base: main
Are you sure you want to change the base?
Conversation
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.
|
1. Code Quality & Structure
2. Functionality
3. Testing Approach
4. Documentation
Areas for Improvement1. 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 2. Backward Compatibility Issue: Removing 3. Linux Support Issue: PR only adds Windows/Mac libraries 4. Test Coverage Observation: Graphics test has known MSYS2 issues (mentioned in PR)
5. Dependencies Issue: .gitignore excludes 6. Security & Performance Observation: Native libraries downloaded from external repo
Reviewer Checklist (Per Thoth Tech Guidelines)[x] Code Quality - Aligns with C# and .NET standards Action ItemsFor Author:
For Reviewers:
|
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 atcoresdk/src/test. Like the current tests, they are launched via a test runner (incoresdk/src/test/Main).Existing contributions
Past team members had made some progress towards this goal:
download-libraries.shscript, used to download the latest stable librariesUpdates
SplashKitSDK.csprojtools/scripts/testcoresdk/src/testtools/scripts/test/Mainused to launch tests.gitignoreupdated to ignore downloaded libraries and intermediate build artifactstools/scripts/test/test.csprojSplashKit.cssymbolic link from NuGet folder, in favor of a compilation link in.csproj. Symbolic links are problematic in MSYS2 - see more here.download-libraries.sh: These suggestions were implemented with adjustments inSplashKitSDK.csproj.Type of change
How has this been tested?
All tests from included test suite successfully run on:
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:
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/testcan be built and run to cross-reference.Checklist