Support building on macOS with optional X11 #46

qmfrederik · 2017-03-15T18:45:00Z

X11 no longer ships with macOS, so building libgdiplus over there fails. This PR makes x11 integration optional, by adding a --without-x11 flag.

This will also enable adoption of libgdiplus in homebrew.

Related issues: Homebrew/legacy-homebrew#45393, #13, #35

This fixes cross-compilation. This code is contributed under the terms of the MIT license.

When X11 is not available (like on recent OS X versions), the DPI is initialized to a default value of 96.

monojenkins · 2017-03-15T18:45:01Z

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

dnfclas · 2017-03-15T18:45:03Z

@qmfrederik,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

qmfrederik · 2017-03-15T18:46:59Z

@akoeplinger could you trigger a build for this? I'd like to double check this does not break the Linux builds

akoeplinger · 2017-03-15T19:07:59Z

whitelist

akoeplinger

Looks fine to me thanks. Note that the tests in libgdiplus are very limited. A good test is to build and install libgdiplus and then run the System.Drawing and WinForms* tests from Mono against it.

*the WinForms tests don't work or crash on Mac.

akoeplinger · 2017-03-15T19:10:53Z

configure.ac

@@ -348,6 +353,37 @@ dnl Test for libpng
 GDIPLUS_LIBS="$GDIPLUS_LIBS $LIBPNG"
 AC_DEFINE(HAVE_LIBPNG, 1, Define if png support is available. Always defined.)

+dnl
+dnl Test for X11. Allow compiling without x11 support using the disable-x11


should be without-x11 since you're using AC_ARG_WITH

akoeplinger · 2017-03-15T19:13:19Z

src/gdiplus-private.h

 	#ifdef CAIRO_HAS_XLIB_SURFACE
 		#include "cairo-xlib.h"
 	#endif
+        #endif


should use tabs instead of spaces here and below.

akoeplinger · 2017-03-15T19:16:52Z

src/graphics-private.h

 	Display			*display;
 	Drawable		drawable;
+#endif
+#endif


this struct isn't passed between managed<->native right? otherwise we might get issues if it changes size/layout.

I checked, it's not in gdipStructs.cs and a search for _Graphics didn't return any struct which matches this one in Mono's codebase, so I'm assuming not.

ok, good. Can you run the System.Drawing tests against the new libgdiplus?

I'll give it a try and let you know how it goes.

qmfrederik · 2017-03-15T23:23:03Z

@akoeplinger Here's how I ran the tests:

Removed /usr/lib/libgdiplus.so* and made sure make run-test returned almost all tests failed (to ensure the tests are using libgdiplus from /usr/lib)
Installed the updated libgdiplus.so into /usr/lib via make install
Ran make run-test and all tests pass:

NUnitLite 1.0.0 (.NET 4.5 Debug)
Copyright 2013, Charlie Poole

Runtime Environment -
    OS Version: Unix 4.4.0.66
  Mono Version: 4.0.30319.42000

............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
** (process:20212): WARNING **: NOT IMPLEMENTED: GdipWidenPath
.................................................................................................................................................................................NOT IMPLEMENTED YET:GdipTransformPointsI (GpGraphics *graphics, GpCoordinateSpace destSpace, GpCoordinateSpace srcSpace, GpPoint *points, int count)
NOT IMPLEMENTED YET:GdipTransformPoints (GpGraphics *graphics, GpCoordinateSpace destSpace 1, GpCoordinateSpace srcSpace 2, GpPointF *points, int count 5)
.................................................................................................................................................................................................................................................................................
** (process:20212): WARNING **: Unknown metafile format: key 1073106242
.......................................................................................................................................................................................................................................................................................................................................................................................................
** (process:20212): WARNING **: Path conversion requested 0 bytes (8388608 x 8388608). Maximum size is 8388608 bytes.
........................................................................................................................................................................................................................................................................................................................................................................................................................................................
Tests run: 1855, Passed: 1845, Errors: 0, Failures: 0, Inconclusive: 0
  Not run: 10, Invalid: 0, Ignored: 10, Skipped: 0
Elapsed time: 00:00:02.0510000

qmfrederik · 2017-03-15T23:24:18Z

@akoeplinger Tried to run the System.Windows.Forms tests by my Linux box is headless so they pretty much all failed because of that.

akoeplinger · 2017-03-15T23:40:14Z

@qmfrederik awesome, thanks for that 👍

Re: WinForms: you can install Xvfb to run it on a headless box, it's how we run it on Jenkins.
I'll do a few more tests on OSX tomorrow but otherwise I think this is good to go.

qmfrederik · 2017-03-16T11:03:03Z

OK, here are the results with xvfb-run:

Tests run: 2850, Passed: 2738, Errors: 0, Failures: 0, Inconclusive: 0
  Not run: 112, Invalid: 0, Ignored: 112, Skipped: 0
Elapsed time: 00:00:11.7690000

qmfrederik · 2017-03-16T18:37:30Z

Related issue: Homebrew/homebrew-core#4202

- Add extra patch that conditionally compiles x11 support [1] - Add X11 option that applies extra patch if X11 support is disabled - PORTREVISION is not bumped as: - The default case (X11) has not changed, and - The non-default case will propogate due to options change [1] mono/libgdiplus#46 PR: 213973 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@438077 35697150-7ecd-e111-bb59-0022644237b5

- Add extra patch that conditionally compiles x11 support [1] - Add X11 option that applies extra patch if X11 support is disabled - PORTREVISION is not bumped as: - The default case (X11) has not changed, and - The non-default case will propogate due to options change [1] mono/libgdiplus#46 PR: 213973

pcercuei and others added 5 commits September 3, 2016 19:02

Use $PKG_CONFIG variable instead of hardcoding pkg-config

f137dd9

This fixes cross-compilation. This code is contributed under the terms of the MIT license.

Add option to disable X11 integration

f1a439f

When X11 is not available (like on recent OS X versions), the DPI is initialized to a default value of 96.

Dont build testgdi if X11 is not available

e6f3273

Fix #if vs #ifdef confusion

2e46f9e

Respect the HAS_X11 flag

d0495c0

dnfclas added the cla-already-signed label Mar 15, 2017

akoeplinger reviewed Mar 15, 2017

View reviewed changes

qmfrederik added 2 commits March 15, 2017 23:16

Fix spacing

36a0a00

PR feedback

402149a

akoeplinger merged commit 910ba3a into mono:master Mar 16, 2017

This was referenced Mar 16, 2017

[V2] Fix cross-compilation and drop dependency towards X11 #13

Closed

Feature/conditional x11 #35

Closed

qmfrederik mentioned this pull request Apr 25, 2017

mono-libgdiplus 5.4 Homebrew/homebrew-core#12862

Closed

4 tasks

qmfrederik deleted the optional-x11 branch April 29, 2017 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support building on macOS with optional X11 #46

Support building on macOS with optional X11 #46

qmfrederik commented Mar 15, 2017

monojenkins commented Mar 15, 2017

dnfclas commented Mar 15, 2017

qmfrederik commented Mar 15, 2017

akoeplinger commented Mar 15, 2017

akoeplinger left a comment

akoeplinger Mar 15, 2017

akoeplinger Mar 15, 2017

akoeplinger Mar 15, 2017

qmfrederik Mar 15, 2017

akoeplinger Mar 15, 2017

qmfrederik Mar 15, 2017

qmfrederik commented Mar 15, 2017

qmfrederik commented Mar 15, 2017

akoeplinger commented Mar 15, 2017

qmfrederik commented Mar 16, 2017

qmfrederik commented Mar 16, 2017

Support building on macOS with optional X11 #46

Support building on macOS with optional X11 #46

Conversation

qmfrederik commented Mar 15, 2017

monojenkins commented Mar 15, 2017

dnfclas commented Mar 15, 2017

qmfrederik commented Mar 15, 2017

akoeplinger commented Mar 15, 2017

akoeplinger left a comment

Choose a reason for hiding this comment

akoeplinger Mar 15, 2017

Choose a reason for hiding this comment

akoeplinger Mar 15, 2017

Choose a reason for hiding this comment

akoeplinger Mar 15, 2017

Choose a reason for hiding this comment

qmfrederik Mar 15, 2017

Choose a reason for hiding this comment

akoeplinger Mar 15, 2017

Choose a reason for hiding this comment

qmfrederik Mar 15, 2017

Choose a reason for hiding this comment

qmfrederik commented Mar 15, 2017

qmfrederik commented Mar 15, 2017

akoeplinger commented Mar 15, 2017

qmfrederik commented Mar 16, 2017

qmfrederik commented Mar 16, 2017