-
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:
|
|
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
This is a universal macOS-native library. It must be included in the 2. Backward Compatibility
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 3. Linux Support
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 4. Test Coverage
The expected failure is now noted in the documentation PR#158. A new ticket has been created to investigate this issue. 5. Dependencies
The 6. Security & Performance
I agree that checksum verification would be an improvement. The deploy process doesn't currently generate a checksum, so the deploy script (
The targeted library versions are simply the "latest" - this is noted in the documentation. |
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