deps: static cares build fails if shared c-ares installed

[sambthompson]

Followup from patches landed in #38, I've had this problem on both Mac OS X 10.9.5 and FreeBSD 10.2:

../deps/cares/src/ares_parse_txt_reply.c:153:25: error: no member named 'record_start' in 'struct ares_txt_reply'
        txt_curr->record_start = strptr == aptr;
        ~~~~~~~~  ^
1 error generated.

Edit, to explain context better: There is a breaking API change in Node's cares to support multiple txt records in dns lookup replies. It has not landed upstream - see discussion in #38. When compiling Node's cares, build is referring to the globally installed headers first, which don't include this change to the struct.

This is immediately caused by -I/usr/local/include appearing before -I../deps/cares/include in the compile command. This, in turn, results from the gyp generated makefile, which orders the includes this way. I note that this is not the order used when compiling V8, where the -I../deps/v8/include parameter appears before -I/usr/local/include.

To reproduce, install c-ares as a shared library in /usr/local/..., then build node with --with-intl=system-icu --without-npm --shared-openssl

In discussion at #38, @jbergstroem suggested this related to other shared libraries being included (e.g. zlib), or it may be related to the use of --with-intl=system-icu

nei;tmm:
The exact steps I used on FreeBSD were:

Download VM-image or liveboot iso [for clean env];
Boot...

# Install c-ares.
pkg install c-ares

# Now try to build node 
pkg install gmake libexecinfo python27 icu openssl

mkdir ~/build && cd ~/build
curl https://iojs.org/dist/latest/iojs-v3.2.0.tar.xz | tar -xf -

setenv CC clang
setenv CXX clang++
setenv LINK clang++
setenv GYP_DEFINES clang=1

./configure --with-intl=system-icu --without-npm --shared-openssl --shared-openssl-includes=usr/local/include/openssl --shared-openssl-libpath=/usr/local/lib

patch deps/v8/src/runtime/runtime-i18n.cc:630
-  local_object->SetInternalField(1, reinterpret_cast<Smi*>(NULL));
+  local_object->SetInternalField(1, reinterpret_cast<Smi*>(0));

make

The patch refered to above is in PR #2636, kindly being shepherded through CI by @thefourtheye. It relates to v8 compilation on FreeBSD, and doesn't otherwise relate to the issue here; just had to patch it to get to this point.

[Fishrock123]

There is a breaking API change in Node's cares to support multiple txt records in dns lookup replies. It has not landed upstream

At this point I'd say we basically are the upstream. :S

[sambthompson]

That's cool. In investigating this, I saw the roliflage in upstream syncing with #1676, and the alternative native js implementation proposed in #1013. It's a tough call between fork-to-own (and spread yourself thinner) vs. roll-your-own (but sit back and enjoy the "reinvent the wheel/not invented here" cracks).

[jbergstroem]

The gist of the issue is that when linking to other shared libraries we will pull include-dirs ahead of the one's we add through our own build which for c-ares then pulls the incorrect headers.

Preferably we're able to land our patch upstream (c-ares). Relevant thread here: http://thread.gmane.org/gmane.network.dns.c-ares/1232/focus=1340

Edit: /CC @indutny (which has done a great job so far)

[saghul]

FWIW, I also patch c-ares in a project of mine, and given the slow rate of changes it has had for a few years now, I'd just go for it. Also trying to upstream everything, of course (as Fedor already did). Personally I was interested in this patch which looks like it will never land because it breaks the ABI. Oh well!

[jbergstroem]

@saghul thanks for your feedback. What do you mean with "go for it"? Tricky part here being us unable to avoid custom include-dirs which would then override ours. Move the custom one after the ones we add?

[sambthompson]

I'm not at all knowledgable about gyp. Is is possible to control/influence include dir order? If so, this is the easiest fix, if upstream patch is a no go. I assume backing out the fix is not acceptable, since node expects to handle multiple text records.

[jbergstroem]

@sambthompson backing out at this stage (1+ year in) is not an option. I'll play around with includedirs but I'm not super keen since this might have bigger consequences.

[sambthompson]

@jbergstroem:

Move the custom one after the ones we add?

For clarity, I though the required fix was to move the custom one (in deps) before the default/global (e.g. /usr/local/include) one.

[sambthompson]

not super keen

I note the include order for v8 is "reversed" (i.e. global after custom) with no ill effects. In fact, I would have assumed this is always desirable, so that local, specific headers trump global ones. Not a C expert though. I've ignored the old <header.h> vs "header.h" convention here.

[saghul]

@saghul thanks for your feedback. What do you mean with "go for it"? Tricky part here being us unable to avoid custom include-dirs which would then override ours. Move the custom one after the ones we add?

@jbergstroem Yep. Those include dirs which Node strictly depends on should probably come first, in order to avoid situations like this. (Note that I haven't looked deeply at how Node handles this currently, this is more of a generic comment.)

[jbergstroem]

@saghul cool. I'll give it a go and see how CI feel; I'll also install cares at a few locations just to make sure. Thanks again for your input.

[sambthompson]

@jbergstroem thanks; please feel free to ping me if you want me to retest in my Mac and BSD envs. I expect if the change is problematic, we'll just go from compile to link errors.

[jbergstroem]

@sambthompson we have to walk cautiously here, we also have a similar behaviour in node-gyp. My preferred route would be (re)activating upstream into uniforming the api but worst case we try something out for a while.

[sambthompson]

Agreed. Let me how I can help.

[sambthompson]

Just noticed sync with upstream c-ares with fix for this [#5199] landed in v5.6.0.