Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the option of building against a shared cares #38

Closed
wants to merge 1 commit into from

Conversation

jbergstroem
Copy link
Member

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.

Bundled cares differs from upstream which will result
in a compilation error when trying to used a shared cares.
@indutny
Copy link
Member

indutny commented Dec 3, 2014

LGTM, @bnoordhuis please take a look too.

@jbergstroem
Copy link
Member Author

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

@bnoordhuis
Copy link
Member

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
Copy link
Contributor

cjihrig commented Dec 3, 2014

Landed with a slightly tweaked commit message in 8868378

@cjihrig cjihrig closed this Dec 3, 2014
cjihrig pushed a commit that referenced this pull request Dec 3, 2014
Bundled cares differs from upstream which will result
in a compilation error when trying to used a shared cares.

Fixes: nodejs/node-v0.x-archive#8786
PR-URL: #38
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@jbergstroem
Copy link
Member Author

@bnoordhuis Here's the difference: a60a9b0

rvagg pushed a commit that referenced this pull request Dec 4, 2014
Bundled cares differs from upstream which will result
in a compilation error when trying to used a shared cares.

Fixes: nodejs/node-v0.x-archive#8786
PR-URL: #38
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2014
Bundled cares differs from upstream which will result
in a compilation error when trying to used a shared cares.

Fixes: nodejs/node-v0.x-archive#8786
PR-URL: #38
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2014
Bundled cares differs from upstream which will result
in a compilation error when trying to used a shared cares.

Fixes: nodejs/node-v0.x-archive#8786
PR-URL: #38
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2014
Bundled cares differs from upstream which will result
in a compilation error when trying to used a shared cares.

Fixes: nodejs/node-v0.x-archive#8786
PR-URL: #38
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg
Copy link
Member

rvagg commented Dec 4, 2014

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
Copy link
Member

@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
Copy link
Member

indutny commented Dec 4, 2014

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

@indutny
Copy link
Member

indutny commented Dec 4, 2014

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

@jbergstroem
Copy link
Member Author

@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.

@jbergstroem jbergstroem deleted the remove_shared_cares branch February 24, 2015 02:17
@posophe
Copy link

posophe commented Aug 8, 2015

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
Copy link
Member Author

@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
Copy link

posophe commented Aug 10, 2015

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
Copy link
Member Author

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

@sambthompson
Copy link

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
Copy link
Member Author

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

@sambthompson
Copy link

@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
Copy link
Member Author

@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
Copy link

@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
Copy link
Member Author

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

giancorderoortiz added a commit to giancorderoortiz/node_backports that referenced this pull request Sep 6, 2024
Original commit message:

    Merged: [parser] Using FunctionParsingScope for parsing class static blocks

    Class static blocks contain statements, don't inherit the
    ExpressionScope stack.

    (cherry picked from commit 3e037e195e508dea045f5626862412e8f64fc919)

    Bug: 341663589
    Change-Id: Ice9a710293b028e5d9fd30d5d85c4842f970b152
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5558360
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Shu-yu Guo <[email protected]>
    Commit-Queue: Adam Klein <[email protected]>
    Cr-Commit-Position: refs/branch-heads/12.4@{nodejs#38}
    Cr-Branched-From: 309640da62fae0485c7e4f64829627c92d53b35d-refs/heads/12.4.254@{nodejs#1}
    Cr-Branched-From: 5dc24701432278556a9829d27c532f974643e6df-refs/heads/main@{#92862}

Refs: v8/v8@6e5e105
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.

7 participants