SixLabors / Fonts

:black_nib: Font loading and layout library.
https://sixlabors.com/products/fonts
Other
308 stars 71 forks source link

Recent commit "Enumerate available fonts through the native API on macOS" breaks UWP native build. #306

Closed woutware closed 2 years ago

woutware commented 2 years ago

Prerequisites

Description

A recent commit "Enumerate available fonts through the native API on macOS" breaks the UWP native/optimized build. The compilation goes ok, but when running, the application will produce the following error message:


UwpConsoleApp.exe - System Error

The code execution cannot proceed because D:\System\Library\Frameworks\CoreFoundation.framework\Versions\A\CoreFoundation.dll was not found. Reinstalling the program may fix this problem.

OK

Steps to Reproduce

Start VS 2019 (not later versions). Then choose menu Extensions -> Manage Extensions, and from Visual Studio Market place add the extension template "Console App (Universal) Project Template...". Also install the "Classic 'New Project' Dialog" extension, and enable it in Tools -> Options -> Classic Project Dialog -> Check the "Use Classic New Project Dialog" . Now you can create an UWP C# console app. Change the build to release mode, x64, make sure the "Compile with .NET Native toolchain" and "Optimize code" checkboxes are checked in the project build properties. Now compile and run, and it should run OK.

Now add a reference to SixLabors.Fonts 1.0.0-beta19. Recompile and run again. Now the error should occur.

Note that the problem does not happen when the "Optimize code" checkbox is not checked. In debug mode the app will run OK.

System Configuration

Windows 10, VS 2019.

The issue first started to happen in 1.0.0-beta17. Version 1.0.0-beta16 is ok. I've further pinpointed the source to be the following commit: https://github.com/SixLabors/Fonts/commit/5bc3f05be1996e1d05b2d44b56434d511b6fd49e.

So when running on windows, UWP will fail because it cannot find the specific MacOS dll.

My first thought is to separate all native calls in a separate nuget package so that you can just omit that nuget package reference when compiling an UWP app.

JimBobSquarePants commented 2 years ago

Wouldn’t that be an issue with the build system then?

Sergio0694 commented 2 years ago

.NET Native doesn't have support for native libraries loading, and instead has the compiler rewrite all P/Invokes to be static imports during application startup from the application package. Because each package is self-contained and the toolchain has full knowledge of what .dll-s will be available, it also knows which calls will never be resolved, like this one. The issue is that missing .dll imports are just rewritten to be throw stubs during application loading, which is why it's crashing at startup for you.

There is no fix as far as I know, other than physically removing those P/Invoke-s from the target that UWP uses. I had to do the same in ComputeSharp too, where I've had to move some P/Invoke-s to a different package entirely, because otherwise UWP would just throw at startup if those were present, even if the code never actually invokes them at runtime.

woutware commented 2 years ago

@JimBobSquarePants I wouldn't have a clue, I have no experience with UWP unfortunately to know what is supposed to be happening. Practically I'm thinking I can get this resolved quicker by separating the native bits in a separate package than getting MS to look into this.

@Sergio0694 Thanks for sharing your knowledge on this, that kind of matches what I'm seeing indeed. The code doesn't even get called, not even the namespace needs to mentioned for this to happen. Just the reference being there will trigger it.

woutware commented 2 years ago

@Sergio0694 I see you're working at MS in a related area. If anything, the error message is kinda terrible, because it doesn't mention the original dll and class that is causing the problem. So you're sent on a hunt to find out which nuget package the error is coming from, and you have to remove all nuget references, add them back again, 1 by 1 and test. And on my machine the native optimized compile takes 15 minutes! So you can imagine the enjoyable experience I was having. Do you think it is worth reporting this to MS to improve the error message and save other developers a lot of time?

Sergio0694 commented 2 years ago

It should alreay be possible to gather more info on this I think. Try compiling your application but set the output diagnostics level to detailed/diagnostics (from the VS options). That will cause the toolchain to spit out a ton more info (you can also just save a binary log and open that to make things simpler). You should see some warning coming from .NET Native about an unresolved P/Invoke or something, which should also indicate what class/method it's coming from 🙂

JimBobSquarePants commented 2 years ago

There is no fix as far as I know, other than physically removing those P/Invoke-s from the target that UWP uses. I had to do the same in ComputeSharp too, where I've had to move some P/Invoke-s to a different package entirely, because otherwise UWP would just throw at startup if those were present, even if the code never actually invokes them at runtime.

This concerns me greatly. I don't have the ability to support multiple Fonts libraries. I'd rather strip the feature entirely.

The issue is that missing .dll imports are just rewritten to be throw stubs during application loading, which is why it's crashing at startup for you.

This seems clearly a bad idea then. Can we get that fixed?

JimBobSquarePants commented 2 years ago

Hold on... @woutware just rereading the issue. Are you saying the build works but running does not?

We have an AppContext switch around the functionality. Would that help?

https://github.com/SixLabors/Fonts/blob/380b6680fab5153edd6dc435c743150bd5a97598/src/SixLabors.Fonts/SystemFontCollection.cs#L77-L78

Sergio0694 commented 2 years ago

"This seems clearly a bad idea then."

It's a consequence of .NET Native using static imports only, which would explode if a .dll is missing, so all such cases are just rewritten to be throw stubs during assembly loading instead. That is, .NET Native doesn't have runtime library loading capability, so if a .dll is missing when the application is built it means there's no way it could ever be available at runtime.

"Can we get that fixed?"

Unfortunately no, not likely. The fix was to support library loading, which was actually implemented internally, but then funding was cut and the entire .NET Native interop team left Microsoft, and now the whole project is now in maintenance mode 🥲

Sergio0694 commented 2 years ago

A potential fix for this could be to multi-target for UAP10.0 as well, and strip those P/Invoke-s there. Of course, I realize it's not ideal and it'd also mean the UWP SDK would be required to build ImageSharp.Fonts, so likely not an option here either.

woutware commented 2 years ago

@Sergio0694 Aha, that's very useful to know, but I'm glad I still figured it out with my noob debugging! Still would be nice if the error message contained a bit more info, doesn't cost any money to make the text a bit more descriptive for those noobs like me who didn't know about the output diagnostics option. But I'll admit I haven't read a lot upon building UWP apps yet.

JimBobSquarePants commented 2 years ago

A potential fix for this could be to multi-target for UAP10.0 as well, and strip those P/Invoke-s there.

I'm 1 of maybe 3 people on the planet who builds this from source so it might be a possibility. I'd like to explore the AppContext route though.

woutware commented 2 years ago

@JimBobSquarePants About the AppContext switch, that did in fact cross my mind, but as the test console app isn't even making any SixLabors.Fonts calls, I thought it was unlikely to make a difference. And from what I understand from @Sergio0694 's comment, the stubs that throw the exception are created at compile time, it's just that the compilation doesn't technically fail, but it still is the source of the problem.

But, I guess I can quickly try it out, I just have to figure out how to set the switch directly without using the attribute.

Sergio0694 commented 2 years ago

"the stubs that throw the exception are created at compile time"

Correct. You can crash even with just a package reference, without even using any of the APIs at all. Or really, all you need is just to have a P/Invoke anywhere in your project, even if completely unused, with a wrong .dll path, and the app will explode 😅

DaZombieKiller commented 2 years ago

In theory, you can work around this by writing your own native library that exports the same functions as the 'real' library.

On macOS this library's exports would simply forward to the real ones, but elsewhere it would export stubs that either do nothing or return errors.

This would satisfy the UWP requirement of all P/Invokes being resolvable without requiring you to multi-target. The major downside is that you now have to maintain a native dependency and compile it for all platforms (unless you used a DllImportResolver to force the new name to resolve to the old one), which comes with its own host of difficulties.

JimBobSquarePants commented 2 years ago

This would satisfy the UWP requirement of all P/Invokes being resolvable without requiring you to multi-target. The major downside is that you now have to maintain a native dependency and compile it for all platforms (unless you used a DllImportResolver to force the new name to resolve to the old one), which comes with its own host of difficulties.

I have no idea how to do this. Nor does it sound like something I'd want to do. 😔

Thinking about it I also do not want to multitarget. I'll end up polyfilling a bunch of stuff for the crappy target. I was actually planning on moving everything to a single .NET 6 target.

UWP sounds like a very dead platform. I wouldn't recommend anyone uses it. Shouldn't you be using WinUI3 now?

woutware commented 2 years ago

@JimBobSquarePants Perhaps a fork is appropriate. I was hoping to avoid it if possible, but maybe it's more practical. My customer is having a substantial code base in UWP, and they're not in the position to switch platforms in a short time frame. So I'm looking for a band aid that will tide them over for now. I can possibly do a fork myself, but I haven't built this package myself, maybe you can provide some insight in how feasible/time consuming it would for me to build a custom fork.

JimBobSquarePants commented 2 years ago

@woutware A fork might be an idea. Easy enough to do as the library builds out-of-the-box with dotnet build/pack as long as you have the submodule ready (which should be automatic)

However... I wonder whether we could replicate whatever the native dll's are doing in managed code?

It kinda looks to me like we should be simply adding a few more folders on MacOS.

woutware commented 2 years ago

@JimBobSquarePants Fully managed sounds like the best option. But I guess an API call would give Apple the freedom to move fonts to other physical locations, but you could always adjust to it or make it configurable.

JimBobSquarePants commented 2 years ago

If they did, we can just keep adding new locations. I doubt it would change that much. I'm gonna investigate adding those folders.

JimBobSquarePants commented 2 years ago

@0xced I'd love to hear your thoughts here if I may since you implemented the initial PR.

0xced commented 2 years ago

Interesting, I'll have a look. I never thought that code with a DllImport attribute carefully protected by if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) would cause an issue on other platforms but here we are !

MichalStrehovsky commented 2 years ago

IIRC if you declare the p/invoke without ExactSpelling=true, .NET Native won't create a hard binding for it. I don't think that flag makes a difference outside Windows anyway.

I can't quickly test it but whoever has UWP tools installed should be able to just add a DllImport to a library that doesn't exist with/without ExactSpelling and see if it makes a difference in being able to launch the app.

Some quick searching seems to indicate you'll get a build time message and a throw at runtime when calling the DllImport without ExactSpelling (which we don't care about because it won't be called).

The backstory is that GetProcAddress was for a long time a banned API in the Store, so .NET Native couldn't use it to lazily resolve the p/invoke. It's either a hard import, or nothing. By the time GetProcAddress became allowed, .NET Native moved to sustained engineering and the feature to add it never reached a release branch.

Sergio0694 commented 2 years ago

"IIRC if you declare the p/invoke without ExactSpelling=true, .NET Native won't create a hard binding for it."

Wait wha- why am I only discovering this now 😭😭😭

I just tried and it does indeed just generate a build warning, but the app launches fine 🥲

.nuget\packages\microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\Microsoft.NetNative.targets(809,5): warning : MCG : warning MCG0007: Unresolved P/Invoke method 'foobarbaz.dll!DummyMethod' for method 'System.Int32 PInvokeTest.PInvoke.DummyMethod(System.Int32, System.Int32)'. Calling this method would throw exception at runtime. Please make sure the P/Invoke either points to a Windows API allowed in UWP applications, or a native DLL that is part of the package. If for some reason your P/Invoke does not satisfy those requirements, please use [DllImport(ExactSpelling=true) to indicate that you understand the implications of using non-UWP APIs.

JimBobSquarePants commented 2 years ago

@MichalStrehovsky Awesome! So, we can drop that attribute property. Slap on a note to ignore any build warnings and carry on with our merry days?

0xced commented 2 years ago

Thank you @MichalStrehovsky for chiming in, I have submitted #309 which should fix this issue in the easiest way possible.

I guess I chose to use ExactSpelling = true in the first place because there's no reason to try any other name variation on macOS. I had no idea that this would be a problem for .NET native. 😲

JimBobSquarePants commented 2 years ago

Thanks everyone. This has been a fantastic community effort! ❤️

woutware commented 2 years ago

@MichalStrehovsky Wow, actual mad lad, what a obscure nugget of information! I just tried it in a simple console app to verify, and it does solve it. Just for others colleagues who ever stumble upon this issue, here's both the not working and working code:

Produces runtime error:

using System;
using System.Runtime.InteropServices;

// This example code shows how you could implement the required main function for a 
// Console UWP Application. You can replace all the code inside Main with your own custom code.

// You should also change the Alias value in the AppExecutionAlias Extension in the 
// Package.appxmanifest to a value that you define. To edit this file manually, right-click
// it in Solution Explorer and select View Code, or open it with the XML Editor.

namespace UwpConsoleApp {
    class Program {
        private const string CoreFoundationFramework = "/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation";

        /// <summary>
        /// Returns the number of values currently in an array.
        /// </summary>
        /// <param name="theArray">The array to examine.</param>
        /// <returns>The number of values in <paramref name="theArray"/>.</returns>
        [DllImport(CoreFoundationFramework, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
        public static extern long CFArrayGetCount(IntPtr theArray);

        static void Main(string[] args) {
            if (args.Length == 0) {
                Console.WriteLine("Hello - no args");
            } else {
                for (int i = 0; i < args.Length; i++) {
                    Console.WriteLine($"arg[{i}] = {args[i]}");
                }
            }
            Console.WriteLine("Press a key to continue: ");
            Console.ReadLine();
        }
    }
}

Does not produce runtime error:

using System;
using System.Runtime.InteropServices;

// This example code shows how you could implement the required main function for a 
// Console UWP Application. You can replace all the code inside Main with your own custom code.

// You should also change the Alias value in the AppExecutionAlias Extension in the 
// Package.appxmanifest to a value that you define. To edit this file manually, right-click
// it in Solution Explorer and select View Code, or open it with the XML Editor.

namespace UwpConsoleApp {
    class Program {
        private const string CoreFoundationFramework = "/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation";

        /// <summary>
        /// Returns the number of values currently in an array.
        /// </summary>
        /// <param name="theArray">The array to examine.</param>
        /// <returns>The number of values in <paramref name="theArray"/>.</returns>
        [DllImport(CoreFoundationFramework, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl, ExactSpelling = false)]
        public static extern long CFArrayGetCount(IntPtr theArray);

        static void Main(string[] args) {
            if (args.Length == 0) {
                Console.WriteLine("Hello - no args");
            } else {
                for (int i = 0; i < args.Length; i++) {
                    Console.WriteLine($"arg[{i}] = {args[i]}");
                }
            }
            Console.WriteLine("Press a key to continue: ");
            Console.ReadLine();
        }
    }
}

@Sergio0694 The only thing I can't find is the build warning message. I did change the Tools -> Options -> Projects and Solutions -> Build and Run -> both MSBuild verbosity options to Diagnostic. And then after the build, the I check out the Build Order window and there's a ton of logging. But the only message I see is the following:

1> P/Invoke 'CFArrayGetCount' with [DllImport(ExactSpelling=true)] resolved to '/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation!CFArrayGetCount'. (TaskId:116)

MichalStrehovsky commented 2 years ago

I guess I chose to use ExactSpelling = true in the first place because there's no reason to try any other name variation on macOS. I had no idea that this would be a problem for .NET native. 😲

FWIW, ExactSpelling doesn't do anything outside Windows (notice the giant ifdef block here).

woutware commented 1 year ago

Am I correct that there hasn't been a nuget release with the fix yet? The last release I see on nuget is from 8 months ago.

JimBobSquarePants commented 1 year ago

There’s builds in our nightlies. I’m looking to do a v1 proper but it’s a lot of work and I get very little help.

woutware commented 1 year ago

Hi Jim,

We're in a bit of a conundrum, I've tried using the nightly from myget, and in my small test application everything works fine (debug and native x64 compile). However my customer is running into a very obscure .NET Native x64 compilation error:

Severity    Code    Description Project File    Line    Suppression State
Error       Method 'BaseGlyphBuilder.SetDecoration(TextDecorations, Vector2, Vector2, float)' will always throw an exception due to the missing method 'GlyphRendererParameters.get_LayoutMode()' in assembly 'SixLabors.Fonts'. There may have been a missing assembly, or a dependency on a more recent Windows SDK release.  LT3 C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\Microsoft.NetNative.targets  809 
Error       ILT0005: 'C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\runtime.win10-x64.microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\x64\ilc\Tools\nutc_driver.exe @"C:\LPI\repos\lt3\lt\obj\x64\Debug\ilc\intermediate\MDIL\LT3.rsp"' returned exit code 1   LT3 C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\Microsoft.NetNative.targets  809 
Error       Internal Compiler Error LT3 C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\Microsoft.NetNative.targets  809 
Error       Method 'BaseGlyphBuilder.SetDecoration(TextDecorations, Vector2, Vector2, float)' will always throw an exception due to the missing method 'GlyphRendererParameters.get_LayoutMode()' in assembly 'SixLabors.Fonts'. There may have been a missing assembly, or a dependency on a more recent Windows SDK release.  LT3 C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\Microsoft.NetNative.targets  809 

The thing is, the compiler appears to be wrong: the specific set of package versions that are referenced should be fine, I'm putting them down here for completeness:

<dependency id="SixLabors.Fonts" version="1.0.0-beta19.18" />
<dependency id="SixLabors.ImageSharp" version="2.1.3" />
<dependency id="SixLabors.ImageSharp.Drawing" version="1.0.0-beta15.13" />

I've verified that property GlyphRendererParameters.LayoutMode is indeed present in SixLabors.Fonts beta19.18.

It looks like there's some older sixlabors.fonts version stuck on my customer's machine somewhere, but they've been looking for days, and they've cleaned out everything multiple times. Also the build log indicates it's correctly resolving the reference to sixlabors.fonts 1.0.0-beta19.18. So very obscure things are going on, possibly a bug in the .NET native tool chain is what I'm thinking.

Anyways, could you manage to get out a nuget release of the sixlabors.fonts? We've been stuck on this build problem for about a week now, and I don't see any thing else we can do to move this forward. It looks like it would also need a SixLabors.ImageSharp.Drawing nuget release?

JimBobSquarePants commented 1 year ago

Hi @woutware

Apologies... I had an open PR to merge support for the latest Fonts into Drawing but was waiting until I had merged https://github.com/SixLabors/Fonts/pull/288

That is merged now so you only need to explicitly reference the following package.

<dependency id="SixLabors.ImageSharp.Drawing" version="1.0.0-beta15.14" />

That will implicitly reference SixLabors.Fonts 1.0.0-beta19.19 and SixLabors.ImageSharp 2.1.3.

I'm very close to a stable release of Fonts (setting a 2 week target from today) so these pain point should disappear.