dotnet / Silk.NET

The high-speed OpenGL, OpenCL, OpenAL, OpenXR, GLFW, SDL, Vulkan, Assimp, WebGPU, and DirectX bindings library your mother warned you about.
https://dotnet.github.io/Silk.NET
MIT License
3.89k stars 378 forks source link

Add forced resigning of SDL2 outputs after stripping #2174

Closed joskuijpers closed 1 month ago

joskuijpers commented 1 month ago

This PR changes the SDL2 build to code-sign at the very end, fixing #2174. The build indicated:

strip: warning: changes being made to the file will invalidate the code signature in: /Users/joskuijpers/Developer/Silk.NET/src/Native/Silk.NET.SDL.Native/runtimes/osx/native/libSDL2-2.0.dylib (for architecture x86_64) strip: warning: changes being made to the file will invalidate the code signature in: /Users/joskuijpers/Developer/Silk.NET/src/Native/Silk.NET.SDL.Native/runtimes/osx/native/libSDL2-2.0.dylib (for architecture arm64)

On Mx systems, all binaries need a valid signature. The build now adds a code sign, and has a new message: libSDL2-2.0.dylib: replacing existing signature

alexrp commented 1 month ago

Looks fine in principle, but I think we need to extend the change to all our other native build scripts where we do stripping. So that'd be Assimp, GLFW, OpenAL Soft, SwiftShader, and Vulkan Loader. They all invoke strip the same way, so should be straightforward.

We potentially also need to change SPIR-V Cross, SPIR-V Reflect, and Shaderc -- they're built differently -- but would need to check that the currently-shipped binaries have valid ad-hoc signatures.

joskuijpers commented 1 month ago

I'll give that a go :)

joskuijpers commented 1 month ago

All the ones you mentioned have a correct signature. They all have -arm64 and -x64 libraries, not using lipo. It seems to me the combination of strip and lipo breaks it. So only SDL needs fixing.

or SDL needs to be aligned with all the others by not LIPOing it at all and keeping two separate binaries. That also then sticks to the osx-arm64 convention used in all other packages,

alexrp commented 1 month ago

I would personally prefer if we switched to thin binaries since that results in smaller RID-specific deployments. When a user does dotnet publish -r osx-arm64, it doesn't seem like there's any good reason that osx-x64 code should be included (or vice versa).

SDL and MoltenVK are the only libraries that we ship fat binaries for, and AFAICT, MoltenVK is only done that way because its Xcode project files are set up to only produce fat binaries for macOS (desktop).

cc @Perksey @Beyley for opinions on this.

joskuijpers commented 1 month ago

I checked out the test build you made: this one verifies for me

joskuijpers commented 1 month ago

I think thin binaries might be better in general. Most macOS releases will ship with both for sure though.

Splitting this up for SDL is I think a bit above me. But I do see that it now uses lipo to join them... so not doing it gives you separate ones? It probably needs more changes in config

Perksey commented 1 month ago

Am I correct in assuming that dotnet can’t publish a universal osx distribution today anyway? If so then yeah it makes sense to thin it out.

alexrp commented 1 month ago

Am I correct in assuming that dotnet can’t publish a universal osx distribution today anyway?

That's my impression. I also previously talked to @agocke on Discord about our (and some other projects's) use of osx as meaning "both osx-arm64 and osx-x64" since I noticed this behavior isn't actually documented anywhere. The conclusion was that this is unsupported behavior that just happens to work.

So, even if we stick to fat binaries (as we probably would for MoltenVK specifically), we should probably stop relying on the osx pseudo-RID anyway.

Perksey commented 1 month ago

Isn’t it documented here? The NuGet side I’d imagine has the same level of documentation most obscure NuGet features have indeed.

How similar to Xamarin is net8.0-mac wrt distribution and build? Can that produce fat bundles? If so, then I recommend keeping the fat binary. Otherwise, go thin all the way, including for MoltenVK as it makes sense to be consistent across the library.

net8.0-ios obviously produces fat bundles so I recommend staying fat here.

alexrp commented 1 month ago

Isn’t it documented here?

I hadn't noticed the explanation in the RID graph section. That would seem to change the calculus here. @agocke can you weigh in on this?

How similar to Xamarin is net8.0-mac wrt distribution and build? Can that produce fat bundles? If so, then I recommend keeping the fat binary. Otherwise, go thin all the way, including for MoltenVK as it makes sense to be consistent across the library.

net8.0-ios obviously produces fat bundles so I recommend staying fat here.

I think the question we should be asking, both for macOS and iOS (etc), is: Given thin binaries for each arch and a configuration to produce a fat bundle, can the tooling produce fat binaries? (I.e. do the lipo -create ... that we currently do.) If yes, then it seems like there's no downside to shipping thin binaries. But I really have no idea what the answer to that question is, nor the ability to test. :slightly_frowning_face:

Regarding MoltenVK, the issue there is that their build system always produces fat binaries AFAICT. But maybe we can just lipo -extract arm64 and lipo -extract x86_64 in that specific case.

Perksey commented 1 month ago

I don't believe any of the dotnet tooling has support for implicitly fattening today (which I agree would be awesome), or at least I don't remember this being something Xamarin could do and I doubt the team have had bandwidth to expand upon that.

agocke commented 1 month ago

use of osx as meaning "both osx-arm64 and osx-x64" since I noticed this behavior isn't actually documented anywhere. The conclusion was that this is unsupported behavior that just happens to work.

The architecture-less RIDs are intended for things that have platform dependency, but not architecture dependency, like managed assets that only work on one OS, or plain files that are only relevant for one OS.

We didn't really intend to load native assets from those directories -- but the system doesn't prohibit it either. It doesn't necessarily seem like something we'd break, so I wouldn't suggest you remove it immediately. This is more of a warning that we didn't really design with this use case in mind.

alexrp commented 1 month ago

Ok, maybe we just maintain status quo then, and go with this PR as-is.

@joskuijpers

All the ones you mentioned have a correct signature. They all have -arm64 and -x64 libraries, not using lipo.

Just for good measure, would you mind checking MoltenVK as well?

joskuijpers commented 1 month ago

So to summarize:

I think the most logical seems to move away from the universal binaries here. SDL is Lipo-joining them, so avoiding that already would solve it there. (as opposed to what this PR now does, which is fix the code sign).

alexrp commented 1 month ago

dotnet can not build self-contained apps using universal binaries, you need to indicate you want to support x64 and arm, so that it adds both, and both put them together in the same app bundle

To be clear: dotnet publish -r <rid> in vanilla netX.Y cannot do universal bundles, but it seems that you can with netX.Y-<os> where os is some Apple OS. So the fat binaries would still be useful there.

alexrp commented 1 month ago

Actually, disregard that previous comment. It seems there's no official support for universal apps: https://github.com/dotnet/sdk/issues/33469

I don't know if this extends to mobile targets?

joskuijpers commented 1 month ago

Ok, maybe we just maintain status quo then, and go with this PR as-is.

@joskuijpers

All the ones you mentioned have a correct signature. They all have -arm64 and -x64 libraries, not using lipo.

Just for good measure, would you mind checking MoltenVK as well?

MoltenVK is fine. Also not using lipo there, nor strip.

joskuijpers commented 1 month ago

Also checked tvOS, which is a static library, universal for arm64 and arm64e. Not code-signed (as this does not make sense for static libs).

Perksey commented 1 month ago

@alexrp Building a macOS universal app is still supported today:

dylan@Dylans-MBP Downloads % file /Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MacOS/MacCatalystApp1 
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MacOS/MacCatalystApp1: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64:Mach-O 64-bit executable arm64]
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MacOS/MacCatalystApp1 (for architecture x86_64): Mach-O 64-bit executable x86_64
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MacOS/MacCatalystApp1 (for architecture arm64):  Mach-O 64-bit executable arm64
dylan@Dylans-MBP Downloads % file /Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libcoreclr.dylib 
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libcoreclr.dylib: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit dynamically linked shared library x86_64] [arm64]
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libcoreclr.dylib (for architecture x86_64):   Mach-O 64-bit dynamically linked shared library x86_64
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libcoreclr.dylib (for architecture arm64):    Mach-O 64-bit dynamically linked shared library arm64
dylan@Dylans-MBP Downloads % file /Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libSDL2-2.0.dylib 
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libSDL2-2.0.dylib: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit dynamically linked shared library x86_64] [arm64]
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libSDL2-2.0.dylib (for architecture x86_64):  Mach-O 64-bit dynamically linked shared library x86_64
/Users/dylan/Documents/RiderProjects/Scratch/MacCatalystApp1/bin/Release/net8.0-macos/MacCatalystApp1.app/Contents/MonoBundle/libSDL2-2.0.dylib (for architecture arm64):   Mach-O 64-bit dynamically linked shared library arm64
// In Program.cs
CoreFoundation.OSLog.Default.Log("hi"); // needed because linker errors pop up otherwise
<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <TargetFramework>net8.0-macos</TargetFramework>
        <OutputType>Exe</OutputType>
        <Nullable>enable</Nullable>
        <ImplicitUsings>true</ImplicitUsings>
        <RuntimeIdentifiers>osx-x64;osx-arm64</RuntimeIdentifiers>
        <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
        <ApplicationId>com.companyname.MacCatalystApp1</ApplicationId>
        <TrimMode>full</TrimMode>
        <SupportedOSPlatformVersion>11.0</SupportedOSPlatformVersion>
    </PropertyGroup>
    <ItemGroup>
      <PackageReference Include="Silk.NET.SDL" Version="2.21.0" />
    </ItemGroup>
    <ItemGroup>
        <SilkExternalPInvoke Include="Silk.NET.SDL.Sdl" />
    </ItemGroup>
</Project>
Perksey commented 1 month ago

Given the above and also that by virtue of supporting NS20 we still technically support older Xamarin use cases even though we've been explicit about our opinions here in our blog, we should probably #StayFat.

alexrp commented 1 month ago

Sheesh, would be nice if there was actually some authoritative (or findable, at least) documentation on this stuff. There's so much conflicting information floating around.