Skip to content

Fix include paths for cairo#574

Merged
TooTallNate merged 1 commit intoAutomattic:masterfrom
LinusU:include-paths
Jul 22, 2015
Merged

Fix include paths for cairo#574
TooTallNate merged 1 commit intoAutomattic:masterfrom
LinusU:include-paths

Conversation

@LinusU
Copy link
Collaborator

@LinusU LinusU commented Jul 2, 2015

This will need some verification before we can merge, but I think that this is the right thing to do.

pkg-config gives the path to a folder that includes the header files for cairo right away. This is different for e.g. fontconfig which gives a path to a folder that contains a single folder named fontconfig.

In the former case #include <cairo.h> should be used, but in the latter case #include <fontconfig/fontconfig.h> should be used.

This patch fixes the cairo includes to point at <cairo.h>.

I believe that the reason why it worked before (on linux at least, it seems to be broken on OS X) is because the search path included /usr/local/include which in turn contained the folder that (called cairo) that pkg-config pointed to.

The last proof for that the correct linking should be <cairo.h> is the FAQ page on Cairo's official website.

This pull request can be tested simply by running the following command:

npm install LinusU/node-canvas#include-paths

@LinusU LinusU mentioned this pull request Jul 2, 2015
@LinusU
Copy link
Collaborator Author

LinusU commented Jul 2, 2015

Just tested this on Ubuntu 13.10 and pkg-config gives /usr/include/cairo which includes the files right away. I thus believe that this patch is correct on linux as well.

@LinusU
Copy link
Collaborator Author

LinusU commented Jul 2, 2015

Hmm, the Travis CI build fails on 0.8 for an unrelated reason. It seems like it can't find nan on npm?

Anyways, it works on the three other instances so this really seems like the right thing to do 👍

@LinusU LinusU changed the title [WIP] src: fix include paths for cairo src: fix include paths for cairo Jul 2, 2015
@LinusU LinusU changed the title src: fix include paths for cairo Fix include paths for cairo Jul 2, 2015
@LinusU
Copy link
Collaborator Author

LinusU commented Jul 6, 2015

@TooTallNate Any change of merging this?

It should fix #572, #514, #217 and also fix installation on Mac OS X with cairo from brew.

@LinusU
Copy link
Collaborator Author

LinusU commented Jul 14, 2015

@TooTallNate Is there anything more you want provided for this to get merged?

@wonderdogone
Copy link

I built this (node-gyp rebuild) and then included in my node V12.6 project and can confirm canvas now working back as normal. Thanks for this.

@wonderdogone wonderdogone mentioned this pull request Jul 15, 2015
@LinusU
Copy link
Collaborator Author

LinusU commented Jul 20, 2015

Rebased on latest master.

The tests now works on Nodejs 0.8 but fails on iojs because it's trying to get the source for iojs from the nodejs website. It fails on the master branch as well and it's not the fault of this pull request.

@TooTallNate I'm sorry for spamming you but I would really like this merged. We are trying to use node-canvas in production but this is a huge blocker for that.

@garthk
Copy link
Contributor

garthk commented Jul 21, 2015

I'm also blocked on this, @TooTallNate, and have to fork two other projects to take a branch dep on @LinusU. Merging would help a great deal.

TooTallNate added a commit that referenced this pull request Jul 22, 2015
@TooTallNate TooTallNate merged commit d790ecf into Automattic:master Jul 22, 2015
@LinusU
Copy link
Collaborator Author

LinusU commented Jul 22, 2015

Thanks 🍻

@TooTallNate Could you please cut a new release that we can depend on as well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants