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.

Sign up for GitHub

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

Jump to bottom

Replace c-ares with getdns #33317

Closed
wants to merge 8 commits into from
Closed

Replace c-ares with getdns #33317

wants to merge 8 commits into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented May 8, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels May 8, 2020
@cjihrig
Copy link
Contributor

cjihrig commented May 8, 2020

I think this requires updating the LICENSE file.

@devsnek devsnek added dns Issues and PRs related to the dns subsystem. and removed meta Issues and PRs related to the general management of the project. labels May 8, 2020
@devsnek devsnek changed the title Replaces C-ARES with getdns Replaces c-ares with getdns May 8, 2020
@devsnek devsnek changed the title Replaces c-ares with getdns Replace c-ares with getdns May 8, 2020
@mscdex
Copy link
Contributor

mscdex commented May 9, 2020

Perhaps it would be good to have a description somewhere of why c-ares is being replaced by this particular library?

@mscdex
Copy link
Contributor

mscdex commented May 9, 2020

Additionally it might be good to have a CITGM run for this.

addaleax
addaleax reviewed May 9, 2020
View reviewed changes
src/node_dns.cc Outdated Show resolved Hide resolved
src/node_dns.cc Outdated Show resolved Hide resolved
src/node_dns.h Outdated Show resolved Hide resolved
@devsnek
Copy link
Member Author

devsnek commented May 9, 2020

@mscdex It will enable us to start checking off the items in #14713 (though this pr is just the replacement, not new features)

devsnek
devsnek commented May 9, 2020
View reviewed changes
deps/getdns/getdns.gyp
'src/tls',
'src/yxml',
],
'libraries': ['-lunbound'],
Copy link
Member Author

Choose a reason for hiding this comment

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

when i try to build this, the linker fails saying all the libunbound functions are undefined /cc @nodejs/node-gyp

Copy link
Member

@bnoordhuis bnoordhuis May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What is your question? libunbound is a dependency of getdns, yes.

I don't want to discourage you but I don't see this PR going anywhere. getdns also depends on libidn or libidn2 (non-optionally, AFAICT) and those are LGPL.

Copy link
Member

Choose a reason for hiding this comment

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

My apologies, I was wrong about libidn being mandatory - it can be disabled by passing -DUSE_LIBIDN2=OFF to cmake.

Node.js already does IDNA conversion, you just need to undo the removal of the toASCII / toUnicode calls in this PR. I could also open an upstream PR to teach it about ICU.

As to libunbound: that's a thornier issue. It's part of unbound and pretty big. sloccount says 112,000 lines of C.

I don't think it'd be easy to break out just the parts we want and this PR is already pretty top-heavy at 86,000 lines added.

Copy link
Member Author

@devsnek devsnek May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

good to know, I left the toASCII in (it's just moved to internal/dns/modern.js).

I guess I can also look into vendoring unbound. what a rabbit hole this has turned into :D

Copy link
Member

Choose a reason for hiding this comment

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

Is current PR blocked by linker fails ?

Copy link
Member Author

Choose a reason for hiding this comment

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

i need to vendor libunbound as well, and i haven't had the time to figure that out. if only we could use submodules and cmake :(

@devsnek devsnek closed this May 10, 2020
@devsnek devsnek deleted the getdns branch May 10, 2020 07:41
@devsnek devsnek restored the getdns branch May 10, 2020 15:11
@devsnek devsnek reopened this May 10, 2020
sam-github
sam-github suggested changes May 10, 2020
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I haven't reviewed in detail, but I'm marking changes requested to ensure that it lands with a doc/guides/maintaining-getdns.md that describes the update procedure.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
jasnell
jasnell reviewed Jun 8, 2020
View reviewed changes
src/node_dns.h
void (*free)(void* ptr, void* data);
void* (*calloc)(size_t num, size_t size, void* data);
void* (*realloc)(void* ptr, size_t size, void* data);
};

This comment was marked as outdated.

Sign in to view

@devsnek
Copy link
Member Author

devsnek commented Jun 8, 2020 •
edited
Loading

Unless someone really wants to try vendoring libunbound with gyp, i'm going to say this is blocked on nodejs/TSC#901

@devsnek devsnek added the blocked PRs that are blocked by other issues or PRs. label Jun 8, 2020
@devsnek devsnek closed this Apr 9, 2021
@jasnell
Copy link
Member

jasnell commented Apr 9, 2021

@devsnek ... I'm sad this never was able to progress.

@devsnek
Copy link
Member Author

devsnek commented Apr 9, 2021

@jasnell there may yet be good news, stay tuned!

@devsnek devsnek deleted the getdns branch April 16, 2021 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen left review comments

@addaleax addaleax addaleax left review comments

@jasnell jasnell jasnell left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

@sam-github sam-github sam-github requested changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@devsnek @cjihrig @mscdex @jasnell @sam-github @bnoordhuis @addaleax @gengjiawen @nodejs-github-bot