Simplify version checks for freetype and libpng. by anntzer · Pull Request #11983 · matplotlib/matplotlib

anntzer · 2018-08-30T13:33:38Z

Currently, setupext.py replicates a lot of work done by the compiler to
check whether header files are present, and whether freetype and libpng
have sufficiently recent versions.

Instead, we can just add a small stub source file at the top of the
extension sources which just tries to include the header and checks the
version macros. If the header is not found, compilation will
immediately abort with foo.h: No such file or directory; if the
version is too old, we can emit an appropriate error message (#pragma message
is supported by all major compilers and allows expanding of
macros in the error message).

Work related to #9737 (the goal being to not list the include paths ourselves).

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

mdboom · 2018-08-30T21:20:24Z

Seems fine, but we lose the ability to display the version in the event of things working, no? Probably not the end of the world, though...

anntzer · 2018-08-30T21:26:00Z

I can add a #pragma message ... displaying the version in the case things are working as well, though I'm not really convinced of the utility of the thing. Let me know if you'd like that.

tacaswell · 2018-08-31T01:57:51Z

Seeing the version number is helpful to make sure it found the right one (ex conda vs system versions).

anntzer · 2018-08-31T06:09:44Z

Sure, I always emit it now. It's no longer at the top but having the compiler do the job of figuring out what file to include (... and not trying to parse freetype.h ourselves just for the purpose of finding its version...) is the whole point of this PR, so...

QuLogic

pkg-config is the defacto standard on Linux, so I don't really agree with not using it here. I can get behind dropping freetype-config as it's not really used in modern systems. And if you want to ignore pkg-config on Windows, I guess that's fine.

QuLogic · 2018-08-31T07:20:38Z

+
+#pragma message "Compiling with FreeType version " \
+  XSTR(FREETYPE_MAJOR) "." XSTR(FREETYPE_MINOR) "." XSTR(FREETYPE_PATCH) "."
+#if FREETYPE_MAJOR << 16 + FREETYPE_MINOR << 8 + FREETYPE_PATCH < 0020300


This is octal; I think you want 0x020300, which would match the bitshifts.

indeed, fixed

QuLogic · 2018-08-31T07:34:02Z

And I'm also confused, because your original post in #9737 said that we should require pkg-config and drop everything else.

anntzer · 2018-08-31T08:17:54Z

The point here is to not parse the output of pkg-config to try and figure out what is the full include path, find ourselves whether the header is present, and parse the freetype version from freetype.h. This is problematic because we try to do this even if pkg-config is not present, which leads to us manually adding "standard" include paths (/usr/include, /usr/local/include), which causes problems with cross-compilation (as reported recently on gitter) or using conda's gcc (which also uses a separate prefix).

The plan is definitely to still use pkg-config/freetype-config/libpng-config to get the correct compile and link flags (in fact that's more or less needed for freetype, as the standard install needs -I$prefix/include/freetype2), but then just shove all that into extra_compile_args/extra_link_args and let the compiler do its job.

tacaswell · 2018-09-02T17:42:51Z

attn @sandrotosi

anntzer · 2018-09-27T07:54:16Z

Re-pinging @QuLogic @sandrotosi after #11983 (comment). Thanks in advance for your input.

QuLogic · 2018-10-04T04:33:53Z

If we're going to ultimately rely on pkg-config, then I don't think the C macro thing is required at all. We can do pkg-config --cflags --libs freetype >= 2.3 and pkg-config --cflags --libs libpng >= 1.2. Though maybe you'll still require the check for Windows?

anntzer · 2018-10-04T12:28:06Z

The macro-checking is useful both for windows, and for unices in case you don't have pkg-config installed but are passing all flags in manually using $CFLAGS/$LDFLAGS/$CPLUS_INCLUDE_PATH/etc.

anntzer · 2018-10-12T09:06:04Z

@QuLogic Are you still rejecting the patch after the discussion above?
Also note that relying on pkg-config to get the freetype version would result in more less informative messages, because the freetype pkg-config (libtool) version is completely unrelated to its release version, see https://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/docs/VERSIONS.TXT#n45 for a table, and embedding a conversion table for these seems completely overkill, especially as we'll have the release versions as macros during the compilation anyways. (But for future reference if this is needed, manually inspecting the 2.3.0 release (our current oldest-supported version) shows that its libtool version is 9.11.3.)

anntzer · 2018-11-24T14:12:18Z

Re-ping @QuLogic (even if you still reject after the discussion above, please leave a comment as to why, thanks!).

QuLogic

I'll approve, tentatively to the pkg-config re-enablement you mentioned you'd do.

Currently, setupext.py replicates a lot of work done by the compiler to check whether header files are present, and whether freetype and libpng have sufficiently recent versions. Instead, we can just add a small stub source file at the top of the extension sources which just tries to include the header and checks the version macros. If the header is not found, compilation will immediately abort with `foo.h: No such file or directory`; if the version is too old, we can emit an appropriate error message (`#pragma message` is supported by all major compilers and allows expanding of macros in the error message).

anntzer · 2018-12-29T17:31:35Z

(rebased and force pushed additional parentheses in #pragma message() to avoid a harmless warning with msvc)

anntzer · 2018-12-29T18:13:38Z

@QuLogic Thanks. To be clear, the end result should look like #13064, where you can see that the check for pkg-config version (using --atleast-version now, instead of parsing ourselves) is present.

tacaswell · 2018-12-30T21:09:28Z

Going to merge this to un-block the stack of 3 PRs (this on, #11964 and #13064)

anntzer added the Build label Aug 30, 2018

tacaswell added this to the v3.1 milestone Aug 30, 2018

tacaswell requested review from QuLogic and mdboom August 30, 2018 16:33

anntzer force-pushed the builddepchecks branch from 63bd68f to bf76c9e Compare August 31, 2018 06:08

QuLogic requested changes Aug 31, 2018

View reviewed changes

anntzer force-pushed the builddepchecks branch from bf76c9e to 4684773 Compare August 31, 2018 08:11

anntzer mentioned this pull request Sep 1, 2018

Simplify extension setup. #11964

Merged

6 tasks

anntzer force-pushed the builddepchecks branch from 4684773 to 29b088c Compare October 4, 2018 12:26

anntzer force-pushed the builddepchecks branch from 29b088c to 3d4feec Compare October 12, 2018 08:52

anntzer mentioned this pull request Dec 14, 2018

Adds macOS and Windows builds to Azure Pipelines #12982

Closed

QuLogic approved these changes Dec 28, 2018

View reviewed changes

anntzer force-pushed the builddepchecks branch from 3d4feec to 923ce72 Compare December 29, 2018 17:30

anntzer mentioned this pull request Dec 29, 2018

Don't explicitly add default include paths to Extensions #13064

Merged

6 tasks

tacaswell merged commit d1060a8 into matplotlib:master Dec 30, 2018

anntzer deleted the builddepchecks branch December 30, 2018 21:43

Uh oh!

Conversation

anntzer commented Aug 30, 2018

PR Summary

PR Checklist

Uh oh!

mdboom commented Aug 30, 2018

Uh oh!

anntzer commented Aug 30, 2018

Uh oh!

tacaswell commented Aug 31, 2018

Uh oh!

anntzer commented Aug 31, 2018

Uh oh!

QuLogic left a comment • 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!

Uh oh!

QuLogic Aug 31, 2018

Choose a reason for hiding this comment

Uh oh!

anntzer Aug 31, 2018

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Aug 31, 2018

Uh oh!

anntzer commented Aug 31, 2018

Uh oh!

tacaswell commented Sep 2, 2018

Uh oh!

anntzer commented Sep 27, 2018

Uh oh!

QuLogic commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer commented Oct 4, 2018

Uh oh!

anntzer commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer commented Nov 24, 2018

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Dec 29, 2018

Uh oh!

anntzer commented Dec 29, 2018

Uh oh!

tacaswell commented Dec 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QuLogic left a comment •

edited

Loading

QuLogic commented Oct 4, 2018 •

edited

Loading

anntzer commented Oct 12, 2018 •

edited

Loading