Skip to content
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

DirectPInvoke with compile time platform check #95319

Closed
vaind opened this issue Nov 28, 2023 · 2 comments
Closed

DirectPInvoke with compile time platform check #95319

vaind opened this issue Nov 28, 2023 · 2 comments
Labels
area-NativeAOT-coreclr question Answer questions and provide assistance, not an issue with source code or documentation.

Comments

@vaind
Copy link

vaind commented Nov 28, 2023

In an .NET library (https://github.com/getsentry/sentry-dotnet), I'm integrating a native depency ((https://github.com/getsentry/sentry-native) as a static library and linking with DirectPInvoke.
For some of the native functions, the .net code needs to call the appropriate function variant based on the platform it's running on being Windows or Linux. The problem is, with the .net library targeting net8.0 TFM, there doesn't seem to be a way to check do the conditional call that is properly resolved at link-time. This results in a link-time error when compiling for Linux, where windows-specific functions are not exported from the native library.

/usr/bin/ld.bfd: obj/Release/net8.0/linux-x64/native/Sentry.Samples.Console.Basic.o: in function `Sentry_Sentry_Native_C__sentry_options_set_database_pathw':
/home/ivan/dev/sentry-dotnet/src/Sentry/Platforms/Native/CFunctions.cs:301: undefined reference to `sentry_options_set_database_pathw'

The relevant c# code is:

        var dir = GetCacheDirectory(options);
        if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
        {
            options.DiagnosticLogger?.LogDebug("Setting native CacheDirectoryPath on Windows: {0}", dir);
            sentry_options_set_database_pathw(cOptions, dir);
        }
        else
        {
            options.DiagnosticLogger?.LogDebug("Setting native CacheDirectoryPath: {0}", dir);
            sentry_options_set_database_path(cOptions, dir);
        }

....

    [DllImport("sentry-native")]
    private static extern void sentry_options_set_database_path(IntPtr options, string path);

    [DllImport("sentry-native")]
    private static extern void sentry_options_set_database_pathw(IntPtr options, [MarshalAs(UnmanagedType.LPWStr)] string path);

Additionally, static linking in consumer applications is achieved with a buildTransitive/Sentry.Native.targets file:

<Project>
  <ItemGroup Condition="$(TargetFramework.StartsWith('net8')) and '$(OutputType)' == 'Exe' And '$(RuntimeIdentifier)' == 'win-x64'">
    <DirectPInvoke Include="sentry-native" />
    <NativeLibrary Include="$(MSBuildThisFileDirectory)..\sentry-native\win-x64\sentry-native.lib" />
    <NativeLibrary Include="dbghelp.lib" />
    <NativeLibrary Include="winhttp.lib" />
  </ItemGroup>

  <ItemGroup Condition="$(TargetFramework.StartsWith('net8')) and '$(OutputType)' == 'Exe' And '$(RuntimeIdentifier)' == 'linux-x64'">
    <DirectPInvoke Include="sentry-native" />
    <NativeLibrary Include="$(MSBuildThisFileDirectory)..\sentry-native\linux-x64\libsentry-native.a" />
    <LinkerArg Include="-lcurl"/>
  </ItemGroup>

  <ItemGroup Condition="$(TargetFramework.StartsWith('net8')) and '$(OutputType)' == 'Exe' And ('$(RuntimeIdentifier)' == 'osx-x64' or '$(RuntimeIdentifier)' == 'osx-arm64')">
    <DirectPInvoke Include="sentry-native" />
    <NativeLibrary Include="$(MSBuildThisFileDirectory)..\sentry-native\osx\libsentry-native.a" />
    <LinkerArg Include="-lcurl"/>
  </ItemGroup>
</Project>

Ideally, RuntimeInformation.IsOSPlatform(OSPlatform.Windows) could be resolved as a compile-time constant and the conditional code could be trimmed completely when running on other platforms.

Or is there another way to achieve this? AFAICT, the only option I have is removing the library-specific DirectPInvoke specification and listing all symbols explicitly, omitting the single windows-specific symbol.

I've tried adding <DirectPInvoke Remove="sentry-native!sentry_options_set_database_pathw" /> to buildTransitive targets file but that doesn't seem to do anything.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 28, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 28, 2023
@teo-tsirpanis
Copy link
Contributor

Have you tried OperatingSystem.IsWindows()?

@vaind
Copy link
Author

vaind commented Nov 28, 2023

Have you tried OperatingSystem.IsWindows()?

So simple and yes, that actually works fine 🤦

thanks!

@vaind vaind closed this as completed Nov 28, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 28, 2023
@teo-tsirpanis teo-tsirpanis added question Answer questions and provide assistance, not an issue with source code or documentation. area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

2 participants