FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

switch from c-ares to getdns by devsnek · Pull Request #38184 · nodejs/node · GitHub

/ node Public

switch from c-ares to getdns - #38184

Closed
devsnek wants to merge 4 commits into
nodejs:masterfrom
devsnek:getdns-v2
ConversationCommits (4)ChecksFiles changed
Closed

switch from c-ares to getdns#38184
devsnek wants to merge 4 commits into
nodejs:masterfrom
devsnek:getdns-v2

Conversation

devsnek commented Apr 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This will allow us to support lots of new things like dns over tls, dnssec, new dns question types, etc.

nodejs-github-bot added build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Apr 10, 2021
devsnek changed the title deps: vendor getdns switch from c-ares to getdns Apr 10, 2021
devsnek added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 10, 2021

devsnek commented Apr 10, 2021

Copy link
Copy Markdown
Member Author

Current status: working on polyfilling the current dns api/behavior as much as possible. although this is semver-major, its nice not to needlessly hurt people.

devsnek force-pushed the getdns-v2 branch 4 times, most recently from d4a63ad to 2839734 Compare April 10, 2021 07:27
jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_dns.cc Outdated
jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_dns.cc Outdated

jasnell Apr 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

adding a using GetDnsContextPointer = DeleteFnPtr<getdns_context, getdns_context_destroy>; would be nice.

jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_dns.cc Outdated

jasnell Apr 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Given that this is semver-major already, I'm curious if instead of doing the JSON conversion here... can we wrap the response in an object and just return that, allowing the user code to decide when to extract the details? Or, is that just too much additional complexity here?

jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_dns.cc Outdated
Comment on lines 159 to 190

jasnell Apr 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would be nice to return MaybeLocal and avoid the ToLocalChecked()

Suggested change
Local<Value> DNSWrap::RegisterTransaction(getdns_transaction_t tid) {
Local<Promise::Resolver> p =
Promise::Resolver::New(env()->context()).ToLocalChecked();
transactions_[tid].Reset(env()->isolate(), p);
return p->GetPromise();
}
MaybeLocal<Value> DNSWrap::RegisterTransaction(getdns_transaction_t tid) {
Local<Promise::Resolver> p;
if (!Promise::Resolve::New(env()->context()).ToLocal(&p))
return Nothing<Value>();
transactions_[tid].Reset(env()->isolate(), p);
return p->GetPromise();
}

jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_dns.cc Outdated

jasnell Apr 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would be great to avoid the ToLocalChecked() in here.

jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_dns.h Outdated
jasnell reviewed Apr 10, 2021
View reviewed changes
Comment thread src/node_util.cc Outdated

jasnell Apr 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Might be good to add this in node_sockaddr instead.

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
targos mentioned this pull request Apr 11, 2021
Closed

This comment has been minimized.

Sign in to view

devsnek commented Apr 12, 2021

Copy link
Copy Markdown
Member Author

Is there anyone who would be able to help me with the build file conversion? I'm running into some weird issues and I also lack knowledge about a lot of gyp stuff.

gengjiawen commented Apr 12, 2021

Copy link
Copy Markdown
Member

src/node_metadata.cc looks still have cares reference.

gengjiawen commented Apr 12, 2021

Copy link
Copy Markdown
Member

Looks like linux is passed now. Not sure the other two platforms.

root@0a77ea91a229 /o/d/node (getdns-v2)# ./out/Release/node -p process.versions
{
  node: '16.0.0-pre',
  v8: '9.0.257.13-node.9',
  uv: '1.41.0',
  zlib: '1.2.11',
  brotli: '1.0.9',
  modules: '93',
  nghttp2: '1.42.0',
  napi: '8',
  llhttp: '2.1.3',
  getdns: '1.6.0',
  openssl: '1.1.1k+quic',
  cldr: '38.1',
  icu: '68.2',
  tz: '2020d',
  unicode: '13.0',
  ngtcp2: '0.1.0-DEV',
  nghttp3: '0.1.0-DEV'
}

devsnek commented Apr 12, 2021

Copy link
Copy Markdown
Member Author

My local linux build was passing even without the node_metadata changes 😅, but it was failing on the ci from saying getdns/version.o is missing, which is very odd to me.

devsnek commented Apr 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Still same issue:

21:59:40 ar: /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/out/Release/obj.target/getdns/deps/getdns/getdns/src/version.o: No such file or directory

gengjiawen commented Apr 12, 2021

Copy link
Copy Markdown
Member

Still same issue:

21:59:40 ar: /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/out/Release/obj.target/getdns/deps/getdns/getdns/src/version.o: No such file or directory

Could this be ccache issue ? cc @richardlau @rvagg

I am on ubuntu 20.04. Clean build works for me now.

jasnell commented Apr 13, 2021

Copy link
Copy Markdown
Member

Should we strip all those out in the name of backwards compat?

I would say yes for sure. The trailing dot is bound to trip up someone.

Have you benchmarked the new implementation compared to the old yet? The use of the microtaskqueue as opposed to SetImmediate in the old impl should have a noticeable impact but I'm a bit concerned about the JSON.parse and Array.prototype.map usage being a bottleneck and offsetting the gains.

gengjiawen commented Apr 13, 2021

Copy link
Copy Markdown
Member

on build: The build works with Ninja on linux and macOS , but not traditional make.

ar: /home/runner/work/node/node/out/Release/obj.target/getdns/deps/getdns/getdns/src/version.o: No such file or directory

@nodejs/gyp Any clue ?

devsnek commented Apr 13, 2021

Copy link
Copy Markdown
Member Author

I don't think make is the issue, I don't use ninja config locally.

devsnek commented Apr 13, 2021

Copy link
Copy Markdown
Member Author

cc @mscdex on 03317f0 these ttl values are far larger than the max ttl (2147483647), is that intentional?

devsnek force-pushed the getdns-v2 branch 2 times, most recently from 688bc19 to ccfbb27 Compare April 13, 2021 02:55

devsnek commented Apr 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@jasnell i did some light benchmarking and it seems like some things are 1-2% faster and some things are 1-2% slower. that being said, i see a lot of room for improvement here if we can make some upstream changes in getdns to improve how data is moved around.

jasnell commented Apr 13, 2021

Copy link
Copy Markdown
Member

Hmm.. I would have hoped for a bit more of an improvement there. I really do suspect we could do better if we could avoid the JSON conversion and parse. It's good that it doesn't regress however.

One other idea I was considering... Rather than dropping cares immediately, getdns can be introduced in parallel. Done right, it could come in as semver minor so that we can get it into 14 and 16 also with an option to choose between the impls. For 17+ cares would be dropped. This gives a bit more time to make further improvements.

gengjiawen commented Apr 13, 2021

Copy link
Copy Markdown
Member

Patch fix windows @devsnek

iff --git a/deps/getdns/configure_file.py b/deps/getdns/configure_file.py
index 7225a7195b..9391b53bb8 100644
--- a/deps/getdns/configure_file.py
+++ b/deps/getdns/configure_file.py
@@ -36,24 +36,24 @@ vars = {
   'HAVE_STDLIB_H': True,
   'HAVE_STRING_H': True,
   'HAVE_TIME_H': True,
-  'HAVE_UNISTD_H': True,
-  'HAVE_FCNTL_H': True,
-  'HAVE_SIGNAL_H': True,
-  'HAVE_SYS_POLL_H': True,
-  'HAVE_POLL_H': True,
+  'HAVE_UNISTD_H': not IS_WINDOWS,
+  'HAVE_FCNTL_H': not IS_WINDOWS,
+  'HAVE_SIGNAL_H': not IS_WINDOWS,
+  'HAVE_SYS_POLL_H': not IS_WINDOWS,
+  'HAVE_POLL_H': not IS_WINDOWS,
   'HAVE_RESOURCE_H': False,
   'HAVE_SYS_TYPES_H': True,
   'HAVE_SYS_STAT_H': True,
   'HAVE_ENDIAN_H': True,
-  'HAVE_NETDB_H': True,
-  'HAVE_ARPA_INET_H': True,
-  'HAVE_NETINET_IN_H': True,
-  'HAVE_NETINET_TCP_H': True,
-  'HAVE_SYS_SELECT_H': True,
-  'HAVE_SYS_SOCKET_H': True,
-  'HAVE_SYS_SYSCTL_H': True,
-  'HAVE_SYS_TIME_H': True,
-  'HAVE_SYS_WAIT_H': True,
+  'HAVE_NETDB_H': not IS_WINDOWS,
+  'HAVE_ARPA_INET_H': not IS_WINDOWS,
+  'HAVE_NETINET_IN_H': not IS_WINDOWS,
+  'HAVE_NETINET_TCP_H': not IS_WINDOWS,
+  'HAVE_SYS_SELECT_H': not IS_WINDOWS,
+  'HAVE_SYS_SOCKET_H': not IS_WINDOWS,
+  'HAVE_SYS_SYSCTL_H': not IS_WINDOWS,
+  'HAVE_SYS_TIME_H': not IS_WINDOWS,
+  'HAVE_SYS_WAIT_H': not IS_WINDOWS,
   'HAVE_WINDOWS_H': IS_WINDOWS,
   'HAVE_WINSOCK_H': IS_WINDOWS,
   'HAVE_WINSOCK2_H': IS_WINDOWS,
@@ -125,21 +125,21 @@ vars = {
   'HAVE_DECL_INET_NTOP': True,
   'HAVE_WIN_DECL_INET_PTON': False,
   'HAVE_WIN_DECL_INET_NTOP': False,
-  'HAVE_DECL_MKSTEMP': True,
+  'HAVE_DECL_MKSTEMP': not IS_WINDOWS,
   'HAVE_DECL_SIGEMPTYSET': True,
   'HAVE_DECL_SIGFILLSET': True,
   'HAVE_DECL_SIGADDSET': True,
-  'HAVE_DECL_STRPTIME': True,
+  'HAVE_DECL_STRPTIME': not IS_WINDOWS,
   'HAVE_DECL_TCP_FASTOPEN': IS_LINUX,
   'HAVE_DECL_TCP_FASTOPEN_CONNECT': IS_LINUX,
   'HAVE_DECL_MSG_FASTOPEN': IS_LINUX,
-  'HAVE_FCNTL': True,
-  'HAVE_GETTIMEOFDAY': True,
+  'HAVE_FCNTL': not IS_WINDOWS,
+  'HAVE_GETTIMEOFDAY': not IS_WINDOWS,
   'HAVE_IOCTLSOCKET': False,
   'HAVE_SIGEMPTYSET': True,
   'HAVE_SIGFILLSET': True,
   'HAVE_SIGADDSET': True,
-  'HAVE_STRPTIME': True,
+  'HAVE_STRPTIME': not IS_WINDOWS,
   'HAVE_SIGSET_T': True,
   'HAVE__SIGSET_T': False,
   'HAVE_BSD_STDLIB_H': IS_BSD,
@@ -167,7 +167,7 @@ vars = {
   'HAVE_EVENT_BASE_FREE': False,
   'DEFAULT_EVENTLOOP': 'select_eventloop',
   'USE_POLL_DEFAULT_EVENTLOOP': False,
-  'STRPTIME_WORKS': True,
+  'STRPTIME_WORKS': not IS_WINDOWS,
   'FD_SETSIZE': False,
   'REQ_DEBUG': False,
   'SCHED_DEBUG': False,
diff --git a/deps/getdns/getdns.gyp b/deps/getdns/getdns.gyp
index 6c64140c5c..b51172e682 100644
--- a/deps/getdns/getdns.gyp
+++ b/deps/getdns/getdns.gyp
@@ -40,6 +40,18 @@
             'getdns/src/compat/strlcpy.c',
           ],
         }],
+        [ 'OS=="win"', {
+          'sources': [
+            'getdns/src/compat/gettimeofday.c',
+            'getdns/src/compat/arc4random.c',
+            'getdns/src/compat/arc4_lock.c',
+            'getdns/src/compat/arc4random_uniform.c',
+            'getdns/src/compat/getentropy_win.c',
+            'getdns/src/compat/strlcpy.c',
+            'getdns/src/compat/mkstemp.c',
+            'getdns/src/compat/strptime.c',
+          ],
+        }],
         [ 'OS=="freebsd"', {
           'defines': [
             '_POSIX_C_SOURCE=200112L',

gengjiawen commented Apr 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I don't think make is the issue, I don't use ninja config locally.

Ninja works on clean build. Make not (need to build two times).

I am thinking we dump the auto generate version.c. Just commit it like we do in nghttp2.

devsnek commented Apr 13, 2021

Copy link
Copy Markdown
Member Author

make works on a clean build for me

devsnek commented Apr 13, 2021

Copy link
Copy Markdown
Member Author

@nodejs/gyp any ideas about these build failures?

gengjiawen commented Apr 14, 2021

Copy link
Copy Markdown
Member

I am thinking we dump the auto generate version.c. Just commit it like we do in nghttp2.

I believe this patch fix linux and macOS build.
gengjiawen@eed6d4b

richardlau reviewed Apr 14, 2021
View reviewed changes
Comment thread deps/getdns/getdns.gyp Outdated
'getdns/src/getdns/getdns.h',
'getdns/src/getdns/getdns_extra.h',
'getdns/src/version.c',
'getdns/getdns.pc',

richardlau Apr 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Usually the outputs would go into <(INTERMEDIATE_DIR) or <(SHARED_INTERMEDIATE_DIR).

devsnek Apr 15, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this is mirroring the output of the cmake config. is it related to the cause of the ci failures?

richardlau Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes.

diff --git a/deps/getdns/getdns.gyp b/deps/getdns/getdns.gyp
index f035598dd0..fb13a83bda 100644
--- a/deps/getdns/getdns.gyp
+++ b/deps/getdns/getdns.gyp
@@ -10,10 +10,12 @@
         'getdns/src/openssl',
         'getdns/src/tls',
         'getdns/src/yxml',
+        '<(SHARED_INTERMEDIATE_DIR)/getdns/src',
       ],
       'direct_dependent_settings': {
         'include_dirs': [
           'getdns/src',
+          '<(SHARED_INTERMEDIATE_DIR)/getdns/src',
         ],
       },
       'conditions': [
@@ -81,11 +83,11 @@
             'getdns/getdns.pc.in',
           ],
           'outputs': [
-            'getdns/src/config.h',
-            'getdns/src/getdns/getdns.h',
-            'getdns/src/getdns/getdns_extra.h',
-            'getdns/src/version.c',
-            'getdns/getdns.pc',
+            '<(SHARED_INTERMEDIATE_DIR)/getdns/src/config.h',
+            '<(SHARED_INTERMEDIATE_DIR)/getdns/src/getdns/getdns.h',
+            '<(SHARED_INTERMEDIATE_DIR)/getdns/src/getdns/getdns_extra.h',
+            '<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c',
+            '<(SHARED_INTERMEDIATE_DIR)/getdns/getdns.pc',
           ],
           'action': [
             'python', '<@(_inputs)', '--target', '<@(_outputs)',
@@ -139,6 +141,7 @@
         'getdns/src/openssl/tls.c',
         'getdns/src/openssl/pubkey-pinning-internal.c',
         'getdns/src/openssl/keyraw-internal.c',
+        '<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c',
       ]
     }
   ]

allows me to build from a clean repository (git clean -fdX). I get 10 test failures but I assume those are not unexpected/being worked out.

devsnek Apr 16, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thank you!

richardlau Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I haven't tested it but it's possible that the non-headers (i.e. getdns/src/version.c and getdns/getdns.pc) could go in <(INTERMEDIATE_DIR) as they probably aren't needed be available to the things linking to getdns (whereas the generated headers are and need to go into the shared area).

jasnell reviewed Apr 15, 2021
View reviewed changes
Comment thread lib/internal/dns/compat.js

class ReqWrap {
constructor(name) {
this[kResource] = new AsyncResource(name);

jasnell Apr 15, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could the ReqWrap itself just extend from AsyncResource?

devsnek Apr 16, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it could, though that would add the AsyncResource api to the req wrap, which is returned from all the various api calls to the user.

devsnek and others added 4 commits April 16, 2021 10:34
deps: vendor getdns
8812cac 
Co-Authored-By: Jiawen Geng <technicalcute@gmail.com>
Co-Authored-By: Richard Lau <rlau@redhat.com>
dns: switch to getdns
2bb19bd
deps: remove c-ares
c35b7e8
rebuild license
270cf9e

devsnek commented Apr 16, 2021

Copy link
Copy Markdown
Member Author

@nodejs/crypto anyone know what's up with this test?

test: U U U U U SSLv2_method U expect U U ERR_TLS_INVALID_PROTOCOL_METHOD
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:87:1)
client undefined
server ERR_TLS_INVALID_PROTOCOL_METHOD
test: U U U U U SSLv3_method U expect U U ERR_TLS_INVALID_PROTOCOL_METHOD
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:89:1)
client undefined
server ERR_TLS_INVALID_PROTOCOL_METHOD
test: U U U U U hokey-pokey U expect U U ERR_TLS_INVALID_PROTOCOL_METHOD
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:97:1)
client undefined
server ERR_TLS_INVALID_PROTOCOL_METHOD
test: U U U U TLSv1.2 TLS1_2_method U expect U U ERR_TLS_PROTOCOL_VERSION_CONFLICT
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:101:1)
client undefined
server ERR_TLS_PROTOCOL_VERSION_CONFLICT
test: U U U TLSv1.2 U TLS1_2_method U expect U U ERR_TLS_PROTOCOL_VERSION_CONFLICT
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:103:1)
client undefined
server ERR_TLS_PROTOCOL_VERSION_CONFLICT
test: U U SSLv2_method U U U U expect U ERR_TLS_INVALID_PROTOCOL_METHOD U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:91:1)
client ERR_TLS_INVALID_PROTOCOL_METHOD
server undefined
test: U U SSLv3_method U U U U expect U ERR_TLS_INVALID_PROTOCOL_METHOD U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:93:1)
client ERR_TLS_INVALID_PROTOCOL_METHOD
server undefined
test: U U hokey-pokey U U U U expect U ERR_TLS_INVALID_PROTOCOL_METHOD U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:95:1)
client ERR_TLS_INVALID_PROTOCOL_METHOD
server undefined
test: U TLSv1.2 TLS1_2_method U U U U expect U ERR_TLS_PROTOCOL_VERSION_CONFLICT U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:105:1)
client ERR_TLS_PROTOCOL_VERSION_CONFLICT
server undefined
test: TLSv1.2 U TLS1_2_method U U U U expect U ERR_TLS_PROTOCOL_VERSION_CONFLICT U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:107:1)
client ERR_TLS_PROTOCOL_VERSION_CONFLICT
server undefined
test: U U U U U TLSv1_1_method U expect U ECONNRESET ERR_SSL_UNSUPPORTED_PROTOCOL
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:188:5)
client ECONNRESET
server ERR_SSL_UNSUPPORTED_PROTOCOL
test: U U U U U TLSv1_method U expect U ECONNRESET ERR_SSL_UNSUPPORTED_PROTOCOL
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:190:5)
client ECONNRESET
server ERR_SSL_UNSUPPORTED_PROTOCOL
test: U U TLSv1_method U U SSLv23_method U expect U ECONNRESET ERR_SSL_UNSUPPORTED_PROTOCOL
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:144:3)
client ECONNRESET
server ERR_SSL_UNSUPPORTED_PROTOCOL
test: U U TLSv1_1_method U U U U expect U ECONNRESET ERR_SSL_UNSUPPORTED_PROTOCOL
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:176:3)
client ECONNRESET
server ERR_SSL_UNSUPPORTED_PROTOCOL
test: U U TLSv1_method U U U U expect U ECONNRESET ERR_SSL_UNSUPPORTED_PROTOCOL
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:178:3)
client ECONNRESET
server ERR_SSL_UNSUPPORTED_PROTOCOL
test: U U TLSv1_1_method U U SSLv23_method U expect U ECONNRESET ERR_SSL_UNSUPPORTED_PROTOCOL
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:142:3)
client ECONNRESET
server ERR_SSL_UNSUPPORTED_PROTOCOL
test: U U SSLv23_method U U TLSv1_method U expect U ERR_SSL_UNSUPPORTED_PROTOCOL ERR_SSL_WRONG_VERSION_NUMBER
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:148:3)
client ERR_SSL_UNSUPPORTED_PROTOCOL
server ERR_SSL_WRONG_VERSION_NUMBER
test: U U U U U U U expect TLSv1.3 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:84:1)
test: TLSv1 TLSv1.3 U TLSv1.2 TLSv1.3 U U expect TLSv1.3 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:243:1)
test: U U SSLv23_method U U TLSv1_1_method U expect U ERR_SSL_UNSUPPORTED_PROTOCOL ERR_SSL_WRONG_VERSION_NUMBER
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:146:3)
client ERR_SSL_UNSUPPORTED_PROTOCOL
server ERR_SSL_WRONG_VERSION_NUMBER
test: TLSv1 TLSv1.3 U TLSv1.3 TLSv1.3 U U expect TLSv1.3 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:242:1)
test: U U TLSv1_2_method U U TLSv1_2_method U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:170:1)
test: U U TLSv1_method U U TLS_method U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:113:1)
test: TLSv1 TLSv1.1 U TLSv1 TLSv1.2 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:231:1)
test: U U TLSv1_1_method TLSv1 TLSv1.2 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:227:1)
test: TLSv1 TLSv1.2 U TLSv1.1 TLSv1.1 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:238:1)
test: TLSv1 TLSv1.1 U TLSv1 TLSv1.3 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:230:1)
test: TLSv1.2 TLSv1.2 U TLSv1 TLSv1.3 U U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:250:1)
test: TLSv1.1 TLSv1.1 U TLSv1 TLSv1.3 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:251:1)
test: U U TLSv1_1_method U U TLSv1_1_method U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:171:1)
test: TLSv1 TLSv1.2 U U U TLSv1_1_method U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:222:1)
test: U U TLSv1_2_method U U SSLv23_method U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:125:3)
test: U U TLS_method U U TLSv1_method U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:116:1)
test: TLSv1 TLSv1.3 U TLSv1 TLSv1.1 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:233:1)
test: TLSv1 TLSv1.3 U TLSv1.2 TLSv1.2 U U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:244:1)
test: U U TLS_method U U TLSv1_2_method U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:114:1)
test: U U TLSv1_1_method U U TLS_method U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:112:1)
test: TLSv1 TLSv1.2 U U U TLSv1_method U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:221:1)
test: TLSv1 TLSv1 U TLSv1 TLSv1.3 U U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:252:1)
test: TLSv1 TLSv1.2 U TLSv1 TLSv1 U U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:235:1)
test: TLSv1 TLSv1.2 U U U TLSv1_2_method U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:223:1)
test: U U TLSv1_method U U TLSv1_method U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:172:1)
test: TLSv1 TLSv1.2 U U U TLS_method U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:224:1)
test: TLSv1 TLSv1.2 U TLSv1 TLSv1.1 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:232:1)
test: TLSv1 TLSv1.3 U TLSv1 TLSv1 U U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:236:1)
test: TLSv1 TLSv1.3 U TLSv1 TLSv1 U U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:246:1)
test: U U TLSv1_method TLSv1 TLSv1.2 U U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:226:1)
test: TLSv1 TLSv1 U TLSv1 TLSv1.1 U U expect TLSv1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:234:1)
test: U U TLSv1_2_method U U TLS_method U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:111:1)
test: TLSv1.1 TLSv1.1 U TLSv1 TLSv1.2 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:237:1)
test: U U TLSv1_2_method TLSv1 TLSv1.2 U U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:228:1)
test: TLSv1 TLSv1.2 U TLSv1 TLSv1.3 U U expect TLSv1.2 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:239:1)
test: U U TLS_method U U TLSv1_1_method U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:115:1)
test: TLSv1 TLSv1.3 U TLSv1.1 TLSv1.1 U U expect TLSv1.1 U U
   (/home/runner/work/node/node/test/parallel/test-tls-min-max-version.js:245:1)
Command: out/Release/node /home/runner/work/node/node/test/parallel/test-tls-min-max-version.js

targos commented Apr 18, 2021

Copy link
Copy Markdown
Member

The build config doesn't work with ninja:

ninja: error: obj/deps/getdns/getdns.ninja:185: multiple rules generate obj/deps/getdns/gen/getdns/src/getdns.version.o [-w dupbuild=err]

targos reviewed Apr 18, 2021
View reviewed changes
Comment thread deps/getdns/getdns.gyp
Comment on lines +144 to +145

'<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c',

targos Apr 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
'<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c',

I think process_outputs_as_sources makes this redundant. Removing the line fixes the ninja build.

targos commented Apr 18, 2021

Copy link
Copy Markdown
Member

On my mac, dns.lookup('localhost') fails with "Queries for the name yielded all negative responses".

targos mentioned this pull request Jul 16, 2021
Closed
targos mentioned this pull request Aug 21, 2021
Closed

devsnek commented Sep 12, 2021

Copy link
Copy Markdown
Member Author

i thought this attempt really might work but getdns hasn't responded to my questions about issues i've found, it seems like the project might not be maintained enough for us to really use :(

devsnek closed this Sep 12, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
addaleax addaleax left review comments
richardlau richardlau left review comments
+1 more reviewer
targos targos left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL