GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — dns: Update to c-ares 1.17.1 by lxdicted · Pull Request #36207 · nodejs/node

Conversation

@lxdicted

@lxdicted lxdicted commented Nov 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. label Nov 21, 2020
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 21, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 21, 2020
@nodejs-github-bot

ghost commented Nov 21, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34504/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lxdicted
lxdicted force-pushed the feature/c-ares-1.17.1 branch from b6ef1e1 to f8de31a Compare November 21, 2020 17:32
@nodejs-github-bot

ghost commented Nov 23, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34518/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lxdicted lxdicted mentioned this pull request Nov 24, 2020
Feature/dns caa #35466
Closed
2 of 4 tasks
@nodejs-github-bot

ghost commented Nov 24, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34534/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Nov 25, 2020
View reviewed changes
Comment thread deps/cares/src/ares_process.c Outdated
Comment on lines 63 to 65

ghost Nov 25, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

When this PR is landed, should this be landed as a separate commit that can be floated as a patch for subsequent updates?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

ghost Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This piece of code is used in other places of c-ares, too. I'm going to provide a pull request for it in c-ares. Don't know how this should be handled in nodejs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lxdicted

ghost commented Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

@richardlau I can't see, how these failing tests relate to this pull request:

12:24:29 not ok 2497 parallel/test-webcrypto-derivebits-hkdf
12:24:29   ---
12:24:29   duration_ms: 122.197
12:24:29   severity: fail
12:24:29   exitcode: -15
12:24:29   stack: |-
12:24:29     timeout
12:24:29   ...
12:24:29 not ok 2498 parallel/test-webcrypto-derivebits-node-dh
12:24:29   ---
12:24:29   duration_ms: 121.988
12:24:29   severity: fail
12:24:29   exitcode: -15
12:24:29   stack: |-
12:24:29     timeout
12:24:29   ...
12:25:44 not ok 2499 parallel/test-webcrypto-derivebits-pbkdf2
12:25:50   ---
12:26:05   duration_ms: 180.679
12:26:05   severity: fail
12:26:05   exitcode: -15
12:26:05   stack: |-
12:26:05     timeout
12:26:05   ...

Help would be very welcome to track this down, if this is related to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lxdicted lxdicted mentioned this pull request Nov 25, 2020
Compile fix taken from https://github.com/nodejs/node/pull/36207 c-ares/c-ares#385
Closed
@nodejs-github-bot

ghost commented Nov 27, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34587/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

ghost commented Nov 30, 2020

Copy link
Copy Markdown
Member

@nodejs/dns
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

ghost commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34636/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

ghost commented Dec 2, 2020 •
edited by Trott
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

CI Internet tests: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/17846/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

ghost commented Dec 2, 2020

Copy link
Copy Markdown
Member

Can you explain the process you did to update this? (Maybe there's a doc somewhere I should be following along with?) I would have expected this to update deps/cares/include/ares_version.h but that file is unchanged.
lxdicted reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lxdicted
lxdicted force-pushed the feature/c-ares-1.17.1 branch from 916e38b to a4902f0 Compare December 6, 2020 13:44
@lxdicted

ghost commented Dec 6, 2020

Copy link
Copy Markdown
Contributor Author

@Trott I've did the following:

  1. downloaded the old integrated version (1.16.1) and diffed its content against node's deps/cares
  2. downloaded the current version (1.17.1), and
  3. copied all files of it into deps/cares/{src,include}.
  4. verified, that the changes only in node (from 1.), are not removed with my commit.
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the dns Issues and PRs related to the dns subsystem. label Dec 9, 2020
@Trott

ghost commented Dec 9, 2020

Copy link
Copy Markdown
Member

@nodejs/tsc Any ideas for domain experts who can review this? I can review it, but I wouldn't feel great about it if there wasn't someone more knowledgable giving it another pair of eyes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

ghost commented Dec 9, 2020

Copy link
Copy Markdown
Member

Note for whoever lands this: Subsystem should probably be deps.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 9, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 9, 2020
@nodejs-github-bot

ghost commented Dec 9, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34876/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

ghost commented Dec 12, 2020

Copy link
Copy Markdown
Member

Internet CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/18052/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Dec 12, 2020
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
codebytere
codebytere approved these changes Dec 12, 2020
View reviewed changes
Trott
Trott approved these changes Dec 14, 2020
View reviewed changes
Danny Sonnenschein
deps: update to c-ares 1.17.1
3bd9b81 
PR-URL: nodejs#36207
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Trott force-pushed the feature/c-ares-1.17.1 branch from a4902f0 to 3bd9b81 Compare December 16, 2020 13:49
@Trott
Trott merged commit 3bd9b81 into nodejs:master Dec 16, 2020
@Trott

ghost commented Dec 16, 2020

Copy link
Copy Markdown
Member

Landed in 3bd9b81
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos mentioned this pull request Dec 22, 2020
v15.5.0 release proposal #36597
Merged
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@richardlau richardlau mentioned this pull request Jul 26, 2021
v12.22.4 proposal #39500
Merged
@BethGriggs BethGriggs mentioned this pull request Aug 3, 2021
deps: reflect c-ares source tree #39653
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Trott Trott Trott approved these changes
@codebytere codebytere codebytere approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. 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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@lxdicted @nodejs-github-bot @Trott @mcollina @codebytere @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.