Enable support to use libgdiplus library on macOS #1403

AlexNDRmac · 2020-10-26T20:24:01Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Fixed error with libgdiplus on macOS (Unable to load DLL 'libgdiplus')
Added cache action to CI workflow for all OS in build matrix

Related issue: #1387

…iplus-fix

CLAassistant · 2020-10-26T20:24:06Z

All committers have signed the CLA.

JimBobSquarePants

Some reference changes required here and a little cleanup. The unit test memory issues are known to us and only appear randomly. They relate to one of our test reference codecs.

.github/workflows/build-and-test.yml

JimBobSquarePants · 2020-10-26T22:23:25Z

.github/workflows/build-and-test.yml

                  git fetch --prune --unshallow
                  git submodule -q update --init --recursive

+            - name: Setup DotNet SDK


There's currently an issue with the setup-dotnet action which is preventing us from using it.

The macOs and Ubuntu images already have the required SDK installed.
https://github.com/actions/virtual-environments/blob/main/images/macos/macos-10.15-Readme.md#language-and-runtime

I'm not sure that is a bug. In my projects I'm also had such problems with netcore 2.x and netcore 3.x.
I can propose one solution for this:

Change build matrix

matrix: framework: - 'netcoreapp2.1' - 'netcoreapp3.1' - 'net472'

Configure matrix options:

matrix: options: - os: ubuntu-latest runtime: -x64 codecov: true - os: macos-latest runtime: -x64 codecov: true - os: windows-latest runtime: -x64 codecov: true exclude: - os: ubuntu-latest framework: 'net472' - os: macOS-latest framework: 'net472'

Change runner:

jobs: Build: name: ${{ matrix.options.os }} ${{ matrix.framework }} runs-on: ${{ matrix.options.os }}

After that - you'll have next builds:

macOS-latest netcoreapp2.1 macOS-latest netcoreapp3.1 ubuntu-latest netcoreapp2.1 ubuntu-latest netcoreapp3.1 windows-latest netcoreapp2.1 windows-latest netcoreapp3.1 windows-latest net472

And little change for actions/setup-dotnet@v1

- name: Setup DotNet SDK 2.1.x if: (runner.os == 'macOS' || runner.os == 'Linux') && matrix.framework == 'netcoreapp2.1' uses: actions/setup-dotnet@v1 with: dotnet-version: "2.1.x" - name: Setup DotNet SDK 3.1.x if: (runner.os == 'macOS' || runner.os == 'Linux') && matrix.framework == 'netcoreapp3.1' uses: actions/setup-dotnet@v1 with: dotnet-version: "3.1.x"

I'd consider it a bug since it used to work well enough until recent updates. I also chatted to the repo owner on Twitter and he encouraged me to raise the issue.

I don't understand the purpose of the exclude: additions here?

I'm not convinced your dotnet-setup filter will work. The application is multi targeted so if you setup 2.1.x then that will update the path and the build will likely fail. (Or does it work on unix for some bizarre reason?)

What I'm trying to get across here is that we want the absolute minimum of changes in order to make this work.

macos-latest on 3.1.x is the only target we need for now and we don't want to be running the tests with code coverage on multiple platforms.

exclude - means, CI will skip some build matrix options which you'd like to do not run.

dotnet-setup filter will always work - it triggered by if condition. I agree that looks ugly, but it works explicitly.

I think, you can toying with all of these when you'll need. So, do you want to drop setup-dotnet action?
And... 6 tests on macOs failed, what I need to do with it? Because these fails not related to libgdiplus

Thanks for the updates. The tests are failing because the test environment is expecting a different reference codec. We need to update the tests and reference codec registration to cater for the new environment.

ImageSharp/tests/ImageSharp.Tests/TestUtilities/Tests/TestEnvironmentTests.cs

Lines 84 to 93 in 120080b

public void GetReferenceDecoder_ReturnsCorrectDecoders_Windows(string fileName, Type expectedDecoderType)

{

if (TestEnvironment.IsLinux)

{

return;

}

IImageDecoder decoder = TestEnvironment.GetReferenceDecoder(fileName);

Assert.IsType(expectedDecoderType, decoder);

}

ImageSharp/tests/ImageSharp.Tests/TestUtilities/TestEnvironment.Formats.cs

Lines 53 to 77 in 120080b

private static Configuration CreateDefaultConfiguration()

{

var cfg = new Configuration(

new JpegConfigurationModule(),

new GifConfigurationModule(),

new TgaConfigurationModule());

// Magick codecs should work on all platforms

IImageEncoder pngEncoder = IsWindows ? (IImageEncoder)SystemDrawingReferenceEncoder.Png : new PngEncoder();

IImageEncoder bmpEncoder = IsWindows ? (IImageEncoder)SystemDrawingReferenceEncoder.Bmp : new BmpEncoder();

cfg.ConfigureCodecs(

PngFormat.Instance,

MagickReferenceDecoder.Instance,

pngEncoder,

new PngImageFormatDetector());

cfg.ConfigureCodecs(

BmpFormat.Instance,

IsWindows ? (IImageDecoder)SystemDrawingReferenceDecoder.Instance : MagickReferenceDecoder.Instance,

bmpEncoder,

new BmpImageFormatDetector());

return cfg;

}

src/ImageSharp/ImageSharp.csproj

tests/ImageSharp.Benchmarks/ImageSharp.Benchmarks.csproj

antonfirsov · 2020-10-29T14:48:05Z

I'm not sure whether the current state is acceptable (minimal workflows knowledge here), but it looks like there were only 6 failing tests ATM.
@AlexNDRmac can you decorate those with ~~[PlatformSpecific(TestPlatforms.OSX)]~~ to see if we can get a clean CI run?
EDIT: I meant [PlatformSpecific(~TestPlatforms.OSX)]

AlexNDRmac · 2020-10-30T10:20:04Z

@antonfirsov pls look at CI. 503 code for myget when restoring packages. I can't restart CI jobs manually to check if all macOS test passed.

antonfirsov · 2020-10-30T10:28:08Z

@AlexNDRmac re-runned, failed again. URI seems to have an extra dot at the end: https://www.myget.org/F/sixlabors/api/v3/index.json.

JimBobSquarePants · 2020-10-30T10:39:05Z

Weird. That ref hasn't changed. I wonder if we should switch to a nuget.config file instead?

https://github.com/AlexNDRmac/ImageSharp/blob/af/macos-libgdiplus-fix/Directory.Build.props#L119

JimBobSquarePants · 2020-10-30T16:03:18Z

Looks like they're having issues
http://status.myget.org/

codecov · 2020-10-31T01:48:05Z

Codecov Report

Merging #1403 (9e27be8) into master (abc1573) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1403      +/-   ##
==========================================
- Coverage   83.08%   82.99%   -0.10%     
==========================================
  Files         707      692      -15     
  Lines       31839    31189     -650     
  Branches     3590     3578      -12     
==========================================
- Hits        26454    25884     -570     
+ Misses       4668     4582      -86     
- Partials      717      723       +6

Flag	Coverage Δ
unittests	`82.99% <ø> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...mageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs	`96.52% <0.00%> (-1.29%)`	⬇️
src/ImageSharp/Common/Helpers/SimdUtils.cs	`65.90% <0.00%> (-0.05%)`	⬇️
...rc/ImageSharp/PixelFormats/Utils/PixelConverter.cs	`100.00% <0.00%> (ø)`
...tions/Generated/Bgr24.PixelOperations.Generated.cs	`100.00% <0.00%> (ø)`
...tions/Generated/Rgb24.PixelOperations.Generated.cs	`100.00% <0.00%> (ø)`
...ions/Generated/Argb32.PixelOperations.Generated.cs	`100.00% <0.00%> (ø)`
...ions/Generated/Bgra32.PixelOperations.Generated.cs	`100.00% <0.00%> (ø)`
...ions/Generated/Rgba32.PixelOperations.Generated.cs	`100.00% <0.00%> (ø)`
...on/GlobalHistogramEqualizationProcessor{TPixel}.cs	`96.42% <0.00%> (ø)`
...onverters/JpegColorConverter.FromGrayScaleBasic.cs
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abc1573...0b35db8. Read the comment docs.

JimBobSquarePants · 2020-11-01T13:32:26Z

IT LIIIIVES! Well done @AlexNDRmac 🚀

antonfirsov

Thank you very much for taking the time to work on this! 🥇 Almost done, just a few things.

When I was doing my experiments in #1302, I had 29 failing tests, now I see only 3 disabled ones. Majority was reporting a minor image difference, in stuff like AffineTransformTests or ProjectiveTransformTests:
https://pipelines.actions.githubusercontent.com/L8wwCFkW4FUCsGOMOcVKecb9uNpo8kI6vyU6NkDyYB5YnVAih3/_apis/pipelines/1/runs/689/signedlogcontent/3?urlExpires=2020-11-01T15%3A28%3A08.8086782Z&urlSigningMethod=HMACV1&urlSignature=nVb2mk%2FFsOwQSPIhd7lDVNYBnRxjNayA9q%2BcYI9Kqc4%3D

Any idea why are they gone now? Have you seen them locally?

antonfirsov · 2020-11-01T15:19:18Z

.github/workflows/build-and-test.yml

+            # - name: Setup nuget cache
+            #   uses: actions/cache@v2
+            #   id: nuget-cache
+            #   with:
+            #       path: ~/.nuget
+            #       key: ${{ runner.os }}-nuget-${{ hashFiles('**/src/ImageSharp/*.csproj') }}
+            #       restore-keys: ${{ runner.os }}-nuget-


Is this something we shall try to re-enable? If not then we should probably delete it.
Or if it might be useful in the future, let's document the purpose of the commented out code (and also the reason why is it commented out).

It's disabled temporary. I'll enable it. If you don't mind, I recommend you to use cache for packages, it's seriously speedup build time.

antonfirsov · 2020-11-01T15:20:42Z

tests/ImageSharp.Tests/Processing/Processors/Transforms/ResizeTests.cs

        }

        [Theory]
+        [PlatformSpecific(~TestPlatforms.OSX)]


@JimBobSquarePants reminder to ourselves that we shall open an issue for the disabled tests when this gets merged.

@AlexNDRmac I suppose you work on Mac. Can you help us by posting the output images saved by these tests on your machine? (tests/Images/ActualOutput/ResizeTests/ResizeFromSourceRectangle_Rgba32_CalliphoraPartial.png). Same for the other 2 disabled tests.

Expected images are under tests/Images/External/ReferenceOutput/ResizeTests/ if interested.

I propose to use save-action and fail scenario. For example: tests on mac failed, then starts build step with condition 'on-fail' and save any details (pictures, logs... etc) as build artifacts. Next, you can download it for investigation :)
I can add this scenario to workflow, it's very simple and useful.

Sounds awesome, let's give it a try!
I wonder though what's simplest: here or in a separate PR?

Do the tests not fail locally or something?

I think, better - is separate PR, because this improvement not related to libgdiplus.

On my Mac some tests failed, but on CI such tests - passed :) I use 10.14.16, but CI using 10.15.x.... and on my Mac some tests kills some dotnet sub-processes... It's very interesting case, maybe I can foud whick component have "magic" influence on tests, but experiments requires some time.

AlexNDRmac · 2020-11-03T16:13:15Z

@antonfirsov pls restart CI... smh strange with builds :) macOS passed, but windows not.

antonfirsov · 2020-11-03T16:14:07Z

@AlexNDRmac done, let's see ..

AlexNDRmac · 2020-11-12T11:35:58Z

I see there is no progress with testing on CI... Why?

antonfirsov · 2020-11-12T11:56:00Z

@AlexNDRmac in the last few days there were problems with GitHub, those seem to be fixed now, actions in other PR-s run nicely now, so there must be an issue with the current workflow setup.

I may remember it wrong but I think there was a point with a green CI run in this PR. Can we get back to the minimum set of changes that address the topic in the title and enable macOS runs? We can then try further improvements in follow-up PR-s.

antonfirsov · 2020-11-23T20:03:24Z

@AlexNDRmac if you are still interested contributing the reference output exporter action to our builds, feel free to do it in a separate PR. (Providing an explanation how to use it!)

Otherwise ... I guess we should close this one, since the original goal has been completed in #1424. Or is there anything else?

AlexNDRmac · 2020-11-23T20:14:11Z

@antonfirsov if you dont mind, I'd like to add cache to CI (it may speedup builds) and add step to store actual images for failed jobs (it can be useful for you to debug or investigate the reason of fail).

antonfirsov · 2020-11-23T20:36:17Z

@AlexNDRmac cool, and thanks for the interest! I'm open for both, but prefer to see those as 2 separate (new) PR-s. Much easier to review them and reason about the changes.

antonfirsov and others added 10 commits August 3, 2020 18:57

add "macos-latest" to build matrix

bb55ac8

try installing "mono-libgdiplus"

5d4cf9e

add "shell: bash"

525fdbd

try 'macos-10.15'

0f076c8

try ${{matrix.options.os}}

f087dee

next hack to try

921aa06

if: startsWith(matrix.options.os, 'macos')

ff14733

Merge remote-tracking branch 'origin/af/macos-ci' into af/macos-libgd…

95dfbd0

…iplus-fix

Add Nuget cache, improve build matrix

eb7e80f

Add support using libgdiplus on macOS

fe06e3b

Fix incompatible package reference for OSX

e587161

JimBobSquarePants requested changes Oct 26, 2020

View reviewed changes

AlexNDRmac added 3 commits October 27, 2020 12:35

Revert syntax for workflow, move osx package to build targets

8527b2c

Remove setup netcore action

902fbaa

Disable nuget packages cache

3fc0b60

AlexNDRmac added 2 commits October 30, 2020 11:48

Do not run windows specific tests on macOS

734aca2

Decorate OSX specific tests

a4bf8b7

Merge branch 'master' into af/macos-libgdiplus-fix

9e27be8

antonfirsov reviewed Nov 1, 2020

View reviewed changes

AlexNDRmac added 2 commits November 2, 2020 09:39

Enable nuget cache, rework redundant shell for all steps

c06de77

Declare powershell in unix way

fd7a759

AlexNDRmac and others added 2 commits November 6, 2020 14:08

Save actual output of Images for failed CI runs

2855437

Merge branch 'master' into af/macos-libgdiplus-fix

2af8cba

Merge branch 'master' into af/macos-libgdiplus-fix

0b35db8

AlexNDRmac mentioned this pull request Nov 12, 2020

Enable support to use libgdiplus library on macOS (splitted) #1424

Merged

AlexNDRmac mentioned this pull request Dec 2, 2020

Add action to save Output Images for failed tests #1452

Merged

AlexNDRmac closed this Dec 2, 2020

AlexNDRmac deleted the af/macos-libgdiplus-fix branch December 2, 2020 12:41

	public void GetReferenceDecoder_ReturnsCorrectDecoders_Windows(string fileName, Type expectedDecoderType)
	{
	if (TestEnvironment.IsLinux)
	{
	return;
	}

	IImageDecoder decoder = TestEnvironment.GetReferenceDecoder(fileName);
	Assert.IsType(expectedDecoderType, decoder);
	}

	private static Configuration CreateDefaultConfiguration()
	{
	var cfg = new Configuration(
	new JpegConfigurationModule(),
	new GifConfigurationModule(),
	new TgaConfigurationModule());

	// Magick codecs should work on all platforms
	IImageEncoder pngEncoder = IsWindows ? (IImageEncoder)SystemDrawingReferenceEncoder.Png : new PngEncoder();
	IImageEncoder bmpEncoder = IsWindows ? (IImageEncoder)SystemDrawingReferenceEncoder.Bmp : new BmpEncoder();

	cfg.ConfigureCodecs(
	PngFormat.Instance,
	MagickReferenceDecoder.Instance,
	pngEncoder,
	new PngImageFormatDetector());

	cfg.ConfigureCodecs(
	BmpFormat.Instance,
	IsWindows ? (IImageDecoder)SystemDrawingReferenceDecoder.Instance : MagickReferenceDecoder.Instance,
	bmpEncoder,
	new BmpImageFormatDetector());

	return cfg;
	}

Uh oh!

Enable support to use libgdiplus library on macOS #1403

Enable support to use libgdiplus library on macOS #1403

Uh oh!

Conversation

AlexNDRmac commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

CLAassistant commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexNDRmac Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antonfirsov commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexNDRmac commented Oct 30, 2020

Uh oh!

antonfirsov commented Oct 30, 2020

Uh oh!

JimBobSquarePants commented Oct 30, 2020

Uh oh!

JimBobSquarePants commented Oct 30, 2020

Uh oh!

codecov bot commented Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants commented Nov 1, 2020

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexNDRmac commented Nov 3, 2020

Uh oh!

antonfirsov commented Nov 3, 2020

Uh oh!

AlexNDRmac commented Nov 12, 2020

Uh oh!

antonfirsov commented Nov 12, 2020

Uh oh!

antonfirsov commented Nov 23, 2020

Uh oh!

AlexNDRmac commented Nov 23, 2020

Uh oh!

antonfirsov commented Nov 23, 2020

Uh oh!

Reviewers

AlexNDRmac commented Oct 26, 2020 •

edited

Loading

CLAassistant commented Oct 26, 2020 •

edited

Loading

AlexNDRmac Oct 27, 2020 •

edited

Loading

antonfirsov commented Oct 29, 2020 •

edited

Loading

codecov bot commented Oct 31, 2020 •

edited

Loading