ppy / SDL3-CS

MIT License
46 stars 12 forks source link

Add dockerfile + generation wrapper script #169

Closed smoogipoo closed 4 weeks ago

smoogipoo commented 1 month ago

Resolves https://github.com/ppy/SDL3-CS/issues/144

Running:

docker build -t 'sdl-gen' .
docker run --rm -v .:/app -w /app -it sdl-gen
Susko3 commented 1 month ago

To make the windows and linux enum definitions almost the same, you can add:

    "--nativeTypeNamesToStrip",
    "unsigned int",

    "--with-type",
    "*=int",

The last one changes all enums to be int by default.


I noticed some issues when running this locally, both on WSL Ubuntu 24.04 and with the provided dockerfile. Could it be a difference with x86_64 and arm64?

Excerpt for how it looks for me:

diff --git a/SDL3-CS/SDL3/ClangSharp/SDL_camera.g.cs b/SDL3-CS/SDL3/ClangSharp/SDL_camera.g.cs
index 062ecc6..b6e242b 100644
--- a/SDL3-CS/SDL3/ClangSharp/SDL_camera.g.cs
+++ b/SDL3-CS/SDL3/ClangSharp/SDL_camera.g.cs
@@ -96,7 +96,7 @@ public static unsafe partial class SDL3
         public static extern SDLBool SDL_GetCameraFormat(SDL_Camera* camera, SDL_CameraSpec* spec);

         [DllImport("SDL3", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
-        public static extern SDL_Surface* SDL_AcquireCameraFrame(SDL_Camera* camera, [NativeTypeName("Uint64 *")] ulong* timestampNS);
+        public static extern SDL_Surface* SDL_AcquireCameraFrame(SDL_Camera* camera, [NativeTypeName("Uint64 *")] uint* timestampNS);

         [DllImport("SDL3", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
         public static extern void SDL_ReleaseCameraFrame(SDL_Camera* camera, SDL_Surface* frame);
diff --git a/SDL3-CS/SDL3/ClangSharp/SDL_error.g.cs b/SDL3-CS/SDL3/ClangSharp/SDL_error.g.cs
index b2cf25a..0560512 100644
--- a/SDL3-CS/SDL3/ClangSharp/SDL_error.g.cs
+++ b/SDL3-CS/SDL3/ClangSharp/SDL_error.g.cs
@@ -35,7 +35,7 @@ public static unsafe partial class SDL3

         [DllImport("SDL3", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
         [return: NativeTypeName("bool")]
-        public static extern SDLBool SDL_SetErrorV([NativeTypeName("const char *")] byte* fmt, [NativeTypeName("va_list")] byte* ap);
+        public static extern SDLBool SDL_SetErrorV([NativeTypeName("const char *")] byte* fmt, [NativeTypeName("va_list")] __va_list_tag* ap);

         [DllImport("SDL3", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
         [return: NativeTypeName("bool")]
smoogipoo commented 1 month ago
  • varargs don't compile, remapping __va_list_tag=byte fixes it The last one changes all enums to be int by default.

Thanks. Added and updated the gist.

  • SDL's Uint64 and Sint64 are mapped to uint and int instead of ulong and long Could it be a difference with x86_64 and arm64?

Yeah, there are definitely platform differences coming from somewhere. These lines fix it for me:

    "--undefine-macro=__LP64__",
    "--undefine-macro=_LP64",

But they're not enough since it doesn't work for you. These come from https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp

It would be nice if we could target a specific platform triple, but I couldn't get that to work. I also tried --additional -m64 as done in https://github.com/dotnet/ClangSharp/blob/main/sources/ClangSharpPInvokeGenerator/Properties/GenerateClang.rsp, but it didn't have any effect...

SDL_main.h for Windows doesn't work and results in an empty .g.cs file (this is also a problem in your gist)

Yeah... Will have to fiddle around with the headers available in that container. Probably some more llvm magic missing from somewhere.

Susko3 commented 1 month ago

SDL_main.h for Windows fails because it tries to import the Windows-only <process.h> header trough this long chain. We could provide a dummy process.h and add it to the import path on non-Windows builds.

graph TD;
    SDL_main.h-->SDL_events.h;
    SDL_events.h-->SDL_joystick.h;
    SDL_events.h-->SDL_audio.h;
    SDL_joystick.h-->SDL_mutex.h;
    SDL_audio.h-->SDL_mutex.h;
    SDL_mutex.h-->SDL_thread.h;
    SDL_thread.h-->process.h;
smoogipoo commented 1 month ago

Ideally I'd like Clang to exist in the exact environment being targeted, but if that's not possible (and I'm not experienced enough with what amounts to cross compilation) then I suppose that would be a reasonable solution.

smoogipoo commented 1 month ago

These lines still seem to be required for me even after #172:

    "--with-type",
    "*=int", # all types should be ints by default
    "--undefine-macro=__LP64__",
    "--undefine-macro=_LP64",

Other than that, I've updated the gist to what it's now down to.

Susko3 commented 1 month ago

Adding a dummy <process.h> file to the include searchpath fixes the SDL_main SDL_PLATFORM_WINDOWS problem when building on linux. Try the diff and add it if it works for you.

diff --git a/SDL3-CS/generate_bindings.py b/SDL3-CS/generate_bindings.py
index d6ea2c1..781abca 100644
--- a/SDL3-CS/generate_bindings.py
+++ b/SDL3-CS/generate_bindings.py
@@ -24,6 +24,7 @@ Example:

 import json
 import pathlib
+import platform
 import re
 import subprocess
 import sys
@@ -365,6 +366,11 @@ def should_skip(solo_headers: list[Header], header: Header):
 def main():
     solo_headers = [make_header_fuzzy(header_name) for header_name in sys.argv[1:]]

+    if platform.system() != "Windows":
+        base_command.extend([
+            "--include-directory", csproj_root / "include"
+        ])
+
     prepare_sdl_source()

     sdl_api = get_sdl_api_dump()
diff --git a/SDL3-CS/include/process.h b/SDL3-CS/include/process.h
new file mode 100644
index 0000000..524d68e
--- /dev/null
+++ b/SDL3-CS/include/process.h
@@ -0,0 +1 @@
+// dummy process.h when building for SDL_PLATFORM_WINDOWS on non-windows systems
Susko3 commented 1 month ago

Ideally I'd like Clang to exist in the exact environment being targeted, but if that's not possible (and I'm not experienced enough with what amounts to cross compilation) then I suppose that would be a reasonable solution.

I've found that cross-compiling works if you apt install the required gcc and point CPLUS_INCLUDE_PATH to the appropriate path.

Example:

sudo apt install gcc-13-aarch64-linux-gnu
export CPLUS_INCLUDE_PATH="$(echo /usr/lib/gcc-cross/*/*/include):/usr/include:${CPLUS_INCLUDE_PATH:-}"

When cross compiling for aarch64, I don't need to add this to fix int 64: (i.e. I get what you get)

    "--remap",
    "Sint64=long",
    "Uint64=ulong",
Susko3 commented 1 month ago

Ideally I'd like Clang to exist in the exact environment being targeted, but if that's not possible (and I'm not experienced enough with what amounts to cross compilation) then I suppose that would be a reasonable solution.

I think I understand now what you mean. When we're building platform agnostic headers, we should just use the system default (or a cross compiler that produces consistent results). But when building platform specific, we should use the appropriate cross compiler target and headers, eg --target=x86_64-w64-mingw32 and CPLUS_INCLUDE_PATH="$(echo /usr/lib/gcc/x86_64-w64-mingw32/*/include)" when building for SDL_PLATFORM_WINDOWS.

Seems complex, and out of scope for this PR.

If you're fine with the process.h hackery, then we can merge this PR and figure this out later.

smoogipoo commented 1 month ago

Those remaps also work for me, have removed the undefines.

When we're building platform agnostic headers, we should just use the system default (or a cross compiler that produces consistent results). But when building platform specific, we should use the appropriate cross compiler target and headers

Seems complex, and out of scope for this PR.

Or, at least a common target. Ideally yeah we'd use the appropriate cross compiler target, but it's only Windows that's causing the issues right now.

But as you said, probably fine for now.

If you're fine with the process.h hackery, then we can merge this PR and figure this out later.

Sure, have confirmed that it works locally, and updated the final gist.


Time provided, I'll improve/optimise the Dockerfile later. Or someone else beats me to it :)

smoogipoo commented 1 month ago

Also I'm fairly certain we don't need CPLUS_INCLUDE_PATH. That's just my inexperience.

We should be able to just --include-directories it, which would make platform-specific headers easier to handle. I.e. this also works for me:

diff --git a/SDL3-CS/generate_bindings.py b/SDL3-CS/generate_bindings.py
index d855256..ecab900 100644
--- a/SDL3-CS/generate_bindings.py
+++ b/SDL3-CS/generate_bindings.py
@@ -252,6 +252,8 @@ base_command = [

     "--file-directory", SDL_include_root,
     "--include-directory", SDL_include_root,
+    "--include-directory", "/usr/include",
+    "--include-directory", "/usr/lib/gcc/aarch64-linux-gnu/13/include",
     "--libraryPath", "SDL3",
     "--methodClassName", "SDL3",
     "--namespace", "SDL",
diff --git a/SDL3-CS/generate_bindings.sh b/SDL3-CS/generate_bindings.sh
index a24ee04..9ee976c 100755
--- a/SDL3-CS/generate_bindings.sh
+++ b/SDL3-CS/generate_bindings.sh
@@ -7,7 +7,7 @@ dotnet tool restore
 dotnet restore --ucr SDL3-CS/SDL3-CS.csproj

 export LD_LIBRARY_PATH="$(echo ~/.nuget/packages/libclang.runtime.*/*/runtimes/*/native):$(echo ~/.nuget/packages/libclangsharp.runtime.*/*/runtimes/*/native):${LD_LIBRARY_PATH:-}"
-export CPLUS_INCLUDE_PATH="$(echo /usr/lib/gcc/*/*/include):/usr/include:${CPLUS_INCLUDE_PATH:-}"
+#export CPLUS_INCLUDE_PATH="$(echo /usr/lib/gcc/*/*/include):/usr/include:${CPLUS_INCLUDE_PATH:-}"

 python3 SDL3-CS/generate_bindings.py "$@"
smoogipoo commented 1 month ago

Need to figure out how to get the canonical include dirs

My suggestion is to install the appropriate packages in the dockerfile and only do the whole cross compiler thing when running the python script under docker. Otherwise, just use the systm libs and let the user provide CPLUS_ if they need (if not provided organically by their system) via their env vars.
That way, we have a guaranteed set of headers included in the image.

But I don't think this is super important to do so I wouldn't waste time on it unless you have nothing better to do :P
Worst case we run this as a CI workflow :)

smoogipoo commented 4 weeks ago

Have done some final touchups to the Dockerfile so it doesn't restore every time. Merge is all yours @Susko3