Remove the option of building against a shared cares

[jbergstroem]

Bundled cares differs from upstream which will result in a compilation error when trying to used a shared cares.

Refs nodejs/node-v0.x-archive#8786.

[indutny]

LGTM, @bnoordhuis please take a look too.

[jbergstroem]

Fwiw, I'll ping upstream so we hopefully can revert this in time.

[bnoordhuis]

LGTM

@jbergstroem What error were you seeing? We're at the latest release, v1.10.0, so I wouldn't have expected build errors.

[cjihrig]

Landed with a slightly tweaked commit message in 8868378

[jbergstroem]

@bnoordhuis Here's the difference: a60a9b0

[rvagg]

sorry all, I've screwed up the commit log a tiny bit trying to avoid merge commits (which I hate having to do so am obviously not an expert at)

[bnoordhuis]

@jbergstroem Oh, so it's our own handiwork.

@indutny Did you send that patch upstream? I couldn't find anything on the c-ares mailing list.

[indutny]

I did, but there was no discussion at all: http://c-ares.haxx.se/mail/c-ares-archive-2014-06/0004.shtml

[indutny]

Oh, I recalled it. There was a discussion, but it has stopped. :)

[jbergstroem]

@bnoordhuis Patch to revert building against shared is my own work, the patch to c-ares was written by @indutny.

@indutny Saw that you asked again, great! If TTL really was the objection hopefully it passes this time. Thing is, since you're breaking the abi I think it might be tricky to land as-is.

[posophe]

I would like to come back on this issue. Actually, providing an app with bundled libraries is illegal. Do you have any news about c-ares upstream ? Will they merge your changes ?

[jbergstroem]

@posophe Illegal? I think you're making premature assumptions on licenses here. Feel free to read the c-ares license and add any insights we might have overseen.

[posophe]

Sorry I didn't mean illegal in a legal meaning (sic). I mean it's not allowed in most distributions to provide a bundled library.

[jbergstroem]

Afaik it [the patch] is yet to be merged.

[sambthompson]

I think this is related to the issue this commit is supposed to fix; I get an error building v3.2.0 on Mac OS X 10.9.5 (via MacPorts), with a c-ares 1.10 shared library already installed by another port:

:info:build /usr/bin/clang '-D_DARWIN_USE_64_BIT_INODE=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_GNU_SOURCE' '-DCARES_STATICLIB' '-DHAVE_CONFIG_H' -I**/opt/local/include** -I**../deps/cares/include** -I../deps/cares/src -I../deps/cares/config/darwin -Os -gdwarf-2 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -fno-strict-aliasing -MMD -MF /opt/local/var/macports/build/_Users_user_Developer_ports_devel_iojs/iojs/work/iojs-v3.2.0/out/Release/.deps//opt/local/var/macports/build/_Users_user_Developer_ports_devel_iojs/iojs/work/iojs-v3.2.0/out/Release/obj.target/cares/deps/cares/src/ares_parse_txt_reply.o.d.raw -Os -c -o /opt/local/var/macports/build/_Users_user_Developer_ports_devel_iojs/iojs/work/iojs-v3.2.0/out/Release/obj.target/cares/deps/cares/src/ares_parse_txt_reply.o ../deps/cares/src/ares_parse_txt_reply.c
:info:build ../deps/cares/src/ares_parse_txt_reply.c:153:25: error: no member named 'record_start' in 'struct ares_txt_reply'
:info:build txt_curr->record_start = strptr == aptr;
:info:build ~~~~~~~~ ^
:info:build 1 error generated.
:info:build make[1]: *** [/opt/local/var/macports/build/_Users_user_Developer_ports_devel_iojs/iojs/work/iojs-v3.2.0/out/Release/obj.target/cares/deps/cares/src/ares_parse_txt_reply.o] Error 1
:info:build make[1]: Leaving directory `/opt/local/var/macports/build/_Users_user_Developer_ports_devel_iojs/iojs/work/iojs-v3.2.0/out'
:info:build make: *** [iojs] Error 2

The order of the includes in the gyp generated makefile causes this (although I couldn't work out how to fix this; sorry).

[jbergstroem]

@sambthompson you should probably let upstream [macports] know that they shouldn't add include-dirs for c-ares.

[sambthompson]

@jbergstroem as far as I can see, this is wholly controlled by the gyp config; the MacPorts portfile makes no attempt to control include-dirs for c-ares. The MacPorts build can be forced by uninstalling the conflicting port, but this shouldn't be required since node's lib is static and local. There was a PR in MacPorts four years ago when a similar API conflict arose between node's c-ares and the upstream c-ares; see https://trac.macports.org/ticket/28066 [mac ports down atm, can access via google cache by searching subject "nodejs: Headers from the c-ares port conflict with the bundled c-ares headers"].

In short, the conclusion by the MacPorts folks at the time was that this was an up-stream [i.e. node] issue. Given that node is shipping the deps like c-ares within the node src, isn't it incumbent on node not to trip over conflicting "globally" installed headers and libraries in node's own makefiles? I assume the global include dir is still needed for other, non c-ares headers, e.g. SSL?

I'm not an expert on all this, so I appreciate your feedback/advice. I just want to get my proverbial ducks arranged before approaching another dev team, particular given their response the last time this arose. I think the issue only went away the last time because the breaking API change node made was pushed upstream, but I could be wrong.

[jbergstroem]

@sambthompson: We removed support building against a shared c-ares a good while ago because our bundled codebase deviated from upstream. Unless you're specifically injecting includepaths, there should be no way of "messing this up". I can't reach macports source tree to see whats going on right now, but I suspect they might be modifying config.gypi after it has been generated.

Edit: What might be going on is that it's building against a shared zlib or similar, which would then conflict should you have c-ares installed. Ugh.

[sambthompson]

@jbergstroem Thanks for the rapid response. I got the source from macports; there's only one patch applied [https://svn.macports.org/repository/macports/trunk/dports/devel/iojs/files/patch-common.gypi.diff]:

--- common.gypi.orig    2014-09-16 17:47:52.000000000 -0500
+++ common.gypi 2014-10-13 22:42:11.000000000 -0500
@@ -207,7 +207,6 @@
           'GCC_ENABLE_PASCAL_STRINGS': 'NO',        # No -mpascal-strings
           'GCC_THREADSAFE_STATICS': 'NO',           # -fno-threadsafe-statics
           'PREBINDING': 'NO',                       # No -Wl,-prebind
-          'MACOSX_DEPLOYMENT_TARGET': '10.5',       # -mmacosx-version-min=10.5
           'USE_HEADERMAP': 'NO',
           'OTHER_CFLAGS': [
             '-fno-strict-aliasing',

Although it is to config.gypi, I can't see how this change impacts include-dirs. This appears to delete a constraint on OS X version, but doesn't do anything else (visible).

Looking at the portfile [https://svn.macports.org/repository/macports/trunk/dports/devel/iojs/Portfile], I can't see any reference to zlib. There are references to:

depends_lib             port:icu \
                        port:python27 \
                        path:lib/libssl.dylib:openssl

That last line represents a switch by MacPorts from OpenSSL to LibreSSL.

Then there are other tweaks to paths, mostly around python:

configure.python ${prefix}/bin/python2.7

configure.args-append   --without-npm
configure.args-append   --shared-openssl
configure.args-append   --shared-openssl-includes=${prefix}/include/openssl
configure.args-append   --shared-openssl-libpath=${prefix}/lib
configure.args-append   --with-intl=system-icu

proc rec_glob {basedir pattern} {
    set files [glob -directory $basedir -nocomplain -type f $pattern]
    foreach dir [glob -directory $basedir -nocomplain -type d *] {
        eval lappend files [rec_glob $dir $pattern]
    }
    return $files
}


post-patch {
    foreach f [concat ${worksrcpath}/configure \
                   ${worksrcpath}/tools/gyp/gyp \
                   ${worksrcpath}/node.gyp \
                   ${worksrcpath}/deps/cares/gyp_cares \
                   ${worksrcpath}/deps/npm/node_modules/node-gyp/gyp/gyp \
                   [rec_glob ${worksrcpath} *.py]] {
        reinplace -locale C "s|/usr/bin/env python|${configure.python}|" ${f}
    }
    foreach f [concat ${worksrcpath}/deps/npm/scripts/relocate.sh \
                   ${worksrcpath}/deps/npm/node_modules/semver/bin/semver \
                   ${worksrcpath}/deps/npm/node_modules/which/bin/which \
                   ${worksrcpath}/deps/npm/test/packages/npm-test-array-bin/bin/array-bin \
                   ${worksrcpath}/deps/npm/test/packages/npm-test-dir-bin/bin/dir-bin \
                   ${worksrcpath}/tools/doc/node_modules/marked/bin/marked \
                   [rec_glob ${worksrcpath} *.js]] {
        reinplace -locale C "s|/usr/bin/env node|${prefix}/bin/node|" ${f}
    }
    foreach gypfile [rec_glob ${worksrcpath} *.gyp] {
        reinplace -locale C "s|'python'|'${configure.python}'|" ${gypfile}
    }
}

I note that for macports, prefix is "/opt/local" rather than the traditional "/usr/local", but as far as I can tell, that's the only difference. I suspect that rebuilding node from src using /usr/local with c-ares installed there would give the same result.

I note that although the Portfile upstream is only upgraded to 2.0.2, I've been succesfully bumping versions through to 3.1.0 [with checksum updates] and re-building. It only stopped working once c-ares got installed.

Edit: Markdown snafu; was using blockquotes instead of GH MD syntax highlighting.

[jbergstroem]

Does the system-icu headers by any chance live in /opt/local/include for you?