search

brianc / node-libpq

Simple, low level native bindings to PostgreSQL's libpq from node.js
112 stars 42 forks source link

Target libc++ instead of libstdc++ #49

Closed toddself closed 7 years ago

toddself commented 7 years ago

libstdc++ has been deprecated, but without specific action, native modules built with node 6.x or earlier still attempt to link to libstdc++.

(See: https://github.com/nodejs/nan/issues/606)

This can be fixed by explicitly declaring the lib and OS X target in the binding file.

brianc commented 7 years ago

Nice..thank you so much for the time you put into this...greatly appreciated! @toddself care to take a look at why this is failing in older versions of node? The travis tests are red for a few builds.

toddself commented 7 years ago

Looks like there's a clang configuration option issue: -fno-delete-null-pointer-checks doesn't exist.

but it's a debian machine -- should it even be using clang? I'll see what I can suss out!

brianc commented 7 years ago

Thank yoooou! Yeah no comprendo on the clang thing. :finnadie:

toddself commented 7 years ago

This is an odd issue alright! It's only when you use clang instead of gcc and only with versions 0.10 and 0.12 of node.

I am able to compile and run the test suite fine with gcc on my local machine (arch linux, postgres 9.6) against 0.10 and 0.12, but when I export CXX=clang++ CC=clang npm test, it fails.

I wonder if there is a way to force gyp to always use gcc in some cases? I suppose that wouldn't be be ok for mac users? gyp is still a rather large mystery to me!

To the internets for help!

toddself commented 7 years ago

Hi! Sorry I have not forgotten this issue -- I'm still puzzled by why it's only LLVM that throws the error, and only on 0.10 and 0.12

The lazy way out of this is to say "do you need to support 0.10 and 0.12". If not, then this is probably a moot issue, or at least can be resolved by asking people to use GCC over LLVM (although when you're on a mac that is a bit of a mountain to climb).

I'll try to keep plugging away at this -- however the issue this PR is trying to resolve isn't a fatal issue, so continuing forward right now without merging this doesn't cause any harm :)

brianc commented 7 years ago

Good point! I'll merge this & remove the older environments from travis.yml. I'm going to officially remove support for node < 4.x lts in the next version of node postgres as well.

toddself commented 7 years ago

yay thanks!