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

Replace vague instructions for Mono with precise ones #1633

Closed
wants to merge 1 commit into from

Conversation

YoshiRulz
Copy link

Description

Updates the section for Mono (.NET Framework on Linux) in the documentation for "cross-platform" users. Though I'm grateful it works at all, I thought since I've put in the effort to get a working and clean build config I'd offer it for others to copy. I apologise for not automating the host RID part; I only use Linux.

@CLAassistant
Copy link

CLAassistant commented May 14, 2024

CLA assistant check
All committers have signed the CLA.

@dlemstra
Copy link
Owner

Thanks for pointing this out and this should be updated. But you should also be able to achieve this by doing this in your .csproj:

<PropertyGroup>
  <MagickCopyNativeLinux>true</MagickCopyNativeLinux>
</PropertyGroup>

This was "recently" added and is not yet documented.

@YoshiRulz
Copy link
Author

That is documented immediately above the Mono section. But it doesn't work, which I'm guessing is because the filename is wrong.

@dlemstra
Copy link
Owner

dlemstra commented May 14, 2024

Whoops... I thought I did not document that yet but it turns out I did. It would be nicer if we could fix this on Mono by patching the Magick.NET.targets file in the publish folder. Maybe we could add and extra property MagickCopyNativeMono that adds lib before %(FileName)%(Extension) when that property is true? Can you check if that would work?

@YoshiRulz
Copy link
Author

I don't know how to test a patched version of a NuGet package.

But if you're willing to change it, was there a reason the filenames were .dll.so for .NET Core in the first place? I thought only Mono used that convention.

@dlemstra
Copy link
Owner

You will need to edit that file in your NuGet cache to test this locally. You can find the folders with dotnet nuget locals all -l. You can then find that .targets file in the folder of this package.

I think I had to add the .dll.so to get DllImport to work. But that was ages ago when the first version of .NET Core was launched. And I have seen people requiring to add lib before the name but I never figured out what was the cause of that. It doesn't seem to be required otherwise everyone that runs this library on linux would need to do this manual step.

@YoshiRulz
Copy link
Author

This works:

-  <ItemGroup Condition="'$(MagickCopyNativeLinux)' == 'true'">
+  <ItemGroup Condition="'$(MagickCopyNativeLinux)' == 'true' OR '$(MagickCopyNativeLinuxForMono)' == 'true'">
     <NativeLinuxDLL Include="$(MSBuildThisFileDirectory)\..\..\runtimes\linux-x64\native\*.so" />
     <None Include="@(NativeLinuxDLL)">
       <Link>%(FileName)%(Extension)</Link>
+      <Link Condition="'$(MagickCopyNativeLinuxForMono)' == 'true'">lib%(FileName)%(Extension)</Link>
       <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
     </None>

As does this:

+  <ItemGroup Condition="'$(MagickCopyNativeMono)' == 'true'">
+    <NativeLinuxDLL Include="$(MSBuildThisFileDirectory)\..\..\runtimes\linux-x64\native\*.so" />
+    <None Include="@(NativeLinuxDLL)">
+      <Link>lib%(FileName)%(Extension)</Link>
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+  </ItemGroup>

@dlemstra
Copy link
Owner

dlemstra commented May 17, 2024

I wonder if we could improve the detection and add the lib automatically:

  <ItemGroup Condition="('$(TargetFrameworkIdentifier)' == '.NETFramework' AND '$(MagickCopyNativeWindows)' != 'false') OR '$(MagickCopyNativeWindows)' == 'true'">
    <NativeWindowsDLL Include="$(MSBuildThisFileDirectory)\..\..\runtimes\win*\native\*.dll" />
    <None Include="@(NativeWindowsDLL)" Condition="'$(GenerateManifests)' != 'true'">
      <Link>%(FileName)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
    <Content Include="@(NativeWindowsDLL)" Condition="'$(GenerateManifests)' == 'true'">
      <Link>%(FileName)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
  </ItemGroup>
  <ItemGroup Condition="'$(MagickCopyNativeLinux)' == 'true'">
    <NativeLinuxDLL Include="$(MSBuildThisFileDirectory)\..\..\runtimes\linux-x64\native\*.so" />
    <None Include="@(NativeLinuxDLL)">
      <Link Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">%(FileName)%(Extension)"</Link>
      <Link Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">lib%(FileName)%(Extension)"</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
    <NativeLinuxArm64DLL Include="$(MSBuildThisFileDirectory)\..\..\runtimes\linux-arm64\native\*.so" />
    <None Include="@(NativeLinuxArm64DLL)">
      <Link>%(FileName)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
  </ItemGroup>

Would this work for you with the following settings:

<PropertyGroup>
  <MagickCopyNativeWindows>false</MagickCopyNativeWindows>
  <MagickCopyNativeLinux>true</MagickCopyNativeLinux>
</PropertyGroup>

This should prevent copying of the windows libraries and only use the .so files with lib prefix.

@YoshiRulz
Copy link
Author

After removing the stray quotes in the <Link/> values, that change also works.

@dlemstra
Copy link
Owner

Thanks for helping out and testing this with me. I will be going to use the latest idea and update the documentation.

@dlemstra
Copy link
Owner

dlemstra commented May 27, 2024

I just pushed a commit that addresses the changes that I proposed in this pull request and that will come available in the next release. Closing this now. And thanks for helping me test this.

@dlemstra dlemstra closed this May 27, 2024
@YoshiRulz
Copy link
Author

Confirmed new method in 13.9.0 works great with Mono.

@YoshiRulz YoshiRulz deleted the mono-docs branch June 1, 2024 08:04
@dlemstra
Copy link
Owner

dlemstra commented Jun 1, 2024

Thanks for coming back and telling me that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants