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

Properly Detect & Handle Musl Systems #1657

Merged
merged 16 commits into from
May 20, 2021
Merged

Properly Detect & Handle Musl Systems #1657

merged 16 commits into from
May 20, 2021

Conversation

mattleibow
Copy link
Contributor

Description of Change

When running under mono/net4x, the musl libraries were copied over the glibc libraries, meaning that nothing worked.

This PR not only stops doing that and copies the musl libraries into a sub-folder, but also does runtime detection of Musl and then picks those ones.

Bugs Fixed

API Changes

Behavioral Changes

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Updated documentation

@mattleibow mattleibow added this to In progress in v2.80.3 via automation Mar 13, 2021
@mattleibow mattleibow changed the title Properly Musl systems Properly Detect & Handle Musl Systems Mar 13, 2021
Comment on lines 81 to 87
if (PlatformConfiguration.IsMusl) {
var muslLib = Path.Combine (root, "musl-" + arch, libWithExt);
if (File.Exists (muslLib)) {
foundPath = muslLib;
return true;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a Musl system, first check the musl-* directories.

switch (cpu)
{
case Architecture.X86:
return AccessCheck("/lib/libc.musl-x86.so.1", 0) == 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for /lib/libc.musl-* seems to be a decent way to detect Musl systems. If all else fails, then we fallback to the existing logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existence of that file does not indicate the libc variant that's used by the current process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 Is there any way to check for Musl? Is there some specific thing that exists in Musl or glibc that we can use to detect? Maybe there is some thing we call that returns a string that says musl or not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I told you that technically one system can have both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could try to DllImport gnu_get_libc_version from libc and see if it throws
EntryPointNotFoundException. If it doesn't, you are definitely running with glibc.

Copy link
Contributor

@kekekeks kekekeks Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, since that check happens at runtime, I'd suggest to expose an API for selecting the proper binary and some SkiaSharp-specific environment variable (e. g. SKIASHARP_LIBC_FLAVOR). So detection logic would look like this:

  1. use API-provided-override if configured
  2. use the value from environment variable if exists
  3. attempt to auto-detect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Let me add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only added 1. as I don't want a library to be reaching out into the environment. This can of course be added by a consuming library or by the final app. However, the API that I am making public is the existing PlatformConfiguration class. This was previously used just to detect the OS, arch or pointer size, however, I added both a IsMusl and IsMuslOverride which can be used to control if the process is glibc or musl:

// use default fallback
PlatformConfiguration.IsMuslOverride = null;
// is Musl
PlatformConfiguration.IsMuslOverride = true;
// is NOT Musl
PlatformConfiguration.IsMuslOverride = false;

If there is ever another libc, we can just add a new IsXlibc and a IsXlibcOverride. Then it still up to the consumer to set the correct values. The recommendation is to only set when you are sure it is one or the other. Although not very high chance we will get another libc, setting it to false when unsure will make the detection checks stop early.

If SkiaSharp is detecting Musl incorrectly, then it should be seen as a bug and hopefully will get reported and fixed so the override should remain mostly unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I am making the API internal for now and can be accessed via reflection. I have tests to make sure it stays.

The reason for this is that this lookup system only exists on mono and we are really starting to move away from that. I added the API for the existing apps that need to control the lookup, but with .NET Core and .NET 5+, all the lookup is done via the runtime features.

And if there is some pluggable system that needs multiple versions of libSkiaSharp in memory, then this is still not an issue because since .NET Core 3.0 there is the NativeLibrary which is the preferred and more correct way to override the loading mechanism: https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.nativelibrary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it should actually be using the AssemblyLoadContext and works with the results of a Publish for a project. I created a sample with SkiaSharp based on the docs: https://github.com/mattleibow/SkiaSharpPluggable

@@ -50,18 +50,20 @@ static PlatformConfiguration ()
Is64Bit = IntPtr.Size == 8;
}

public static bool? IsMuslOverride { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically there are other libc implementations, so I'd suggest to have something like

enum LibcFlavor
{
    Glibc,
    Musl
}

Copy link
Contributor Author

@mattleibow mattleibow Mar 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so I changed the whole way of selecting a variant/flavor.

There is now a property LinuxFlavor that can be used to control the whole thing instead of the specific IsMusl. This is useful in cases where there needs to be a specific version of Linux. For example if you need a custom build for say FreeBSD. You can set the flavor at app startup following any checks that you do and then the lookup will use that first.

If the property is null, then I just do the basic glibc/musl check - or rather I check for glibc and assume musl otherwise. If a specific flavor is set, then I do nothing and just use that. This is just a plain string that allows for extension later on, and also is just the name of the folder to use.

Example usage would be on a x64 machine:

  • LinuxFlavor == null with glibc => 'x64'
  • LinuxFlavor == null with non-glibc => 'musl-x64'
  • LinuxFlavor == freebsd => 'freebsd-x64'

Since this is totally customizable, you can even do specific checks for versions and set the flavor to opensuse.14 and then it will look in opensuse.14-arm64 if it were running on an ARM64 machine.

@mattleibow mattleibow merged commit 8b44e79 into main May 20, 2021
v2.80.3 automation moved this from In progress to Done May 20, 2021
@mattleibow mattleibow deleted the dev/musl branch May 20, 2021 07:09
@@ -192,7 +191,7 @@ private static class Mac

private static class Linux
{
private const string SystemLibrary = "libdl.so";
private const string SystemLibrary = "dl";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattleibow apparently this throws DllNotFoundException on a somewhat clean Ubuntu 20.04 installation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v2.80.3
  
Done
Development

Successfully merging this pull request may close these issues.

[BUG] musl binaries overwrite ones built for the normal glibc
2 participants