deps: reintroduce supporting shared c-ares builds by jbergstroem · Pull Request #5775 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

jbergstroem

Pull Request check-list

Affected core subsystem(s)

build, deps

Description of change

As of cc192f0e we've now in sync with upstream which means that we also can allow our users to build against a shared version of c-ares.

Note: It is still up to users to make sure that the library version is on par with what Node.js bundles.

This "reverts" commit 25fa5c4.

Tested on gentoo with a shared version c-ares. Tests pass.

/CC @indutny, @bnoordhuis

@jbergstroem

@indutny

LGTM if it works. Thank you!

@jbergstroem

(@nodejs/build should probably be CC:ed as well)

@mscdex mscdex added dns

Issues and PRs related to the dns subsystem.

c++

Issues and PRs that require attention from people who are familiar with C++.

labels

Mar 18, 2016

bnoordhuis

@@ -8,6 +8,7 @@
'node_has_winsdk%': 'false',
'node_shared_zlib%': 'false',
'node_shared_http_parser%': 'false',
'node_shared_libcares%': 'false',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We previously called it node_shared_cares and it might be nice to keep that name in the (admittedly unlikely) case that people check for process.config.variables.node_shared_cares.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason it's changed is that we now use the shared configure_library so I have to map it to what the .pc file contains. Should be fixable though.

@jasnell

@jbergstroem

@bnoordhuis just pushed a version that uses the old name.

To test this "properly" you likely have to build c-ares with ./configure -enable-symbol-hiding; otherwise c-ares will suffix the exposed api. In short: the homebrew version won't work.

@jbergstroem

Here's some output from a gentoo-system, (built with: ./configure --shared-cares --shared-zlib --shared-openssl --shared-http-parser --shared-libuv):

(staging) ~/node-v5.9.0 $ ldd node linux-vdso.so.1 (0x00007ffef2ba9000) libz.so.1 => /lib64/libz.so.1 (0x00007fe083064000) libhttp_parser.so.2.6.2 => /usr/lib64/libhttp_parser.so.2.6.2 (0x00007fe082e5c000) libuv.so.1 => /usr/lib64/libuv.so.1 (0x00007fe082c38000) librt.so.1 => /lib64/librt.so.1 (0x00007fe082a30000) libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fe082814000) libnsl.so.1 => /lib64/libnsl.so.1 (0x00007fe0825fc000) libdl.so.2 => /lib64/libdl.so.2 (0x00007fe0823f8000) libcares.so.2 => /usr/lib64/libcares.so.2 (0x00007fe0821e6000) libssl.so.1.0.0 => /usr/lib64/libssl.so.1.0.0 (0x00007fe081f70000) libcrypto.so.1.0.0 => /usr/lib64/libcrypto.so.1.0.0 (0x00007fe081b34000) libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libstdc++.so.6 (0x00007fe0817b2000) libm.so.6 => /lib64/libm.so.6 (0x00007fe0814bc000) libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libgcc_s.so.1 (0x00007fe0812a5000) libc.so.6 => /lib64/libc.so.6 (0x00007fe080f09000) /lib64/ld-linux-x86-64.so.2 (0x00007fe08327a000) (staging) ~/node-v5.9.0 $ make check make -C out BUILDTYPE=Release V=1 make[1]: Entering directory '/home/jbergstroem/node-v5.9.0/out' make[1]: Nothing to be done for 'all'. make[1]: Leaving directory '/home/jbergstroem/node-v5.9.0/out' ln -fs out/Release/node node Running main() from gtest_main.cc [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from UtilTest [ RUN ] UtilTest.ListHead [ OK ] UtilTest.ListHead (0 ms) [----------] 1 test from UtilTest (0 ms total)

[----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (0 ms total) [ PASSED ] 1 test. /usr/lib/python-exec/python2.7/python tools/test.py --mode=release message parallel sequential -J [01:39|% 100|+ 1046|- 0]: Done

bnoordhuis

configure_library('libcares', output)
# stay backwards compatible with shared cares builds
output['variables']['node_shared_cares'] = \
output['variables'].pop('node_shared_libcares')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four space indent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a lot of stuff doing 2. Should we perhaps pep8 configure in a separate PR?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no fan of lint commits, but I like new code to have proper indentation.

@jbergstroem

@jbergstroem jbergstroem added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Mar 29, 2016

@saghul

FWIW, upstream does not include this patch: 72c5458

@jbergstroem

@saghul yeah, its known. Thanks for the notice though.

Edit: the reason at least I don't care much is because we don't build shared libraries on Windows.

@jbergstroem

@bnoordhuis

LGTM. Sorry for the delay.

@jbergstroem

@jbergstroem

As of cc192f0e we've now in sync with upstream which means that we also can allow our users to build against a shared version of c-ares.

Note: It is still up to users to make sure that the library version is on par with what Node.js bundles.

This "reverts" commit 25fa5c4.

PR-URL: nodejs#5775 Reviewed-By: Fedor Indutny fedor@indutny.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl

jasnell added a commit that referenced this pull request

Apr 26, 2016

@jasnell

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

jasnell added a commit that referenced this pull request

Apr 26, 2016

@jasnell

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

jasnell added a commit that referenced this pull request

Apr 26, 2016

@jasnell

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

jasnell added a commit that referenced this pull request

Apr 26, 2016

@jasnell

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

jasnell added a commit that referenced this pull request

Apr 26, 2016

@jasnell

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

Labels

build

Issues and PRs related to build files or the CI.

c++

Issues and PRs that require attention from people who are familiar with C++.

dns

Issues and PRs related to the dns subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.