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

deps: update c-ares to 1.18.1 #40660

Closed
wants to merge 2 commits into from
Closed

Conversation

richardlau
Copy link
Member

Add a script to automate updating of the c-ares dependency and accompanying maintenance guide and use the script to update to c-ares 1.18.1.


I've opted to strip out the c-ares tests for space reasons (3 mb, and we didn't include them before). Other than that the script minimize the differences between what is in our source tree compared to the upstream c-ares tar balls, which does mean things being added that we did not have before (e.g. c-ares docs).

@nodejs-github-bot nodejs-github-bot added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Oct 29, 2021
@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. lts-watch-v12.x and removed cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Oct 29, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2021
@nodejs-github-bot

This comment has been minimized.

@richardlau richardlau added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 29, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2021
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM if CI is green.

We'll want to add this to the tools.yml file in #40644.

@Trott
Copy link
Member

Trott commented Oct 29, 2021

We'll want to add this to the tools.yml file in #40644.

Oh wait, no we won't because this is a dependency rather than a tool.

@nodejs-github-bot
Copy link
Collaborator

doc/guides/maintaining-c-ares.md Outdated Show resolved Hide resolved
doc/guides/maintaining-c-ares.md Outdated Show resolved Hide resolved
Comment on lines 5 to 7
1. Unpacking the source in a temporary workspace directory.
1. Removing the `test` directory (to save disk space).
1. Copying over the existing `.gitignore`, pre-generated `config` directory and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Unpacking the source in a temporary workspace directory.
1. Removing the `test` directory (to save disk space).
1. Copying over the existing `.gitignore`, pre-generated `config` directory and
2. Unpacking the source in a temporary workspace directory.
3. Removing the `test` directory (to save disk space).
4. Copying over the existing `.gitignore`, pre-generated `config` directory and

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we want to do that. It's a feature of markdown to write ordered lists only with ones, and I like it because it allows to add an item anywhere in the list without having te renumber everything after it.

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've run the formatter and committed the results (plus the comma change and reworded the bit about LICENSE updates). I kind of agree with @targos re. the GitHub flavored markdown feature but I'm not going to fight the formatter.

Copy link
Member

Choose a reason for hiding this comment

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

@targos @richardlau This is a bit of a spaces vs. tabs kind of thing I suppose, but for what it's worth, I think it's better to format it this way so that the raw markdown is more easily readable. It's supposed to be one of the advantages of markdown that it is useable/understandable in its raw format. It makes things slightly harder for the author, but I think when the differences are small, we should favor the reader. (That said, I haven't looked to see if there is a configuration option in remark to change this behavior. There might be.)

Copy link
Member

Choose a reason for hiding this comment

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

(And I also don't actually feel strongly about it. We do more writing markdown than reading the raw markdown probably, so... I'm just going to submit to the formatter... ¯\(ツ)/¯ )

Comment on lines 9 to 11
1. Replacing the existing `deps/cares` with the workspace directory.
1. Modifying the `cares.gyp` file for file additions/deletions.
1. Rebuilding the main Node.js `LICENSE`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Replacing the existing `deps/cares` with the workspace directory.
1. Modifying the `cares.gyp` file for file additions/deletions.
1. Rebuilding the main Node.js `LICENSE`.
5. Replacing the existing `deps/cares` with the workspace directory.
6. Modifying the `cares.gyp` file for file additions/deletions.
7. Rebuilding the main Node.js `LICENSE`.

./tools/update-cares.sh x.y.z
```

e.g.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
e.g.
e.g.

./tools/license-builder.sh
```

If the `LICENSE` is updated for changes other than this c-ares update those
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If the `LICENSE` is updated for changes other than this c-ares update those
If the `LICENSE` is updated for changes other than this c-ares update, those

git add -A deps/cares
```

Add the rebuilt `LICENSE` if it has been updated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add the rebuilt `LICENSE` if it has been updated.
Add the rebuilt `LICENSE` if it has been updated.

targos pushed a commit that referenced this pull request Nov 6, 2021
Add a script to automate updating of the c-ares dependency and
accompanying maintenance guide.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Nov 6, 2021
Updated as described in doc/guides/maintaining-c-ares.md.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos targos mentioned this pull request Nov 8, 2021
richardlau added a commit that referenced this pull request Nov 24, 2021
Add a script to automate updating of the c-ares dependency and
accompanying maintenance guide.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
richardlau added a commit that referenced this pull request Nov 24, 2021
Updated as described in doc/guides/maintaining-c-ares.md.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
richardlau added a commit that referenced this pull request Nov 25, 2021
Add a script to automate updating of the c-ares dependency and
accompanying maintenance guide.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
richardlau added a commit that referenced this pull request Nov 25, 2021
Updated as described in doc/guides/maintaining-c-ares.md.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@richardlau richardlau mentioned this pull request Nov 25, 2021
BethGriggs pushed a commit that referenced this pull request Nov 25, 2021
Add a script to automate updating of the c-ares dependency and
accompanying maintenance guide.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this pull request Nov 25, 2021
Updated as described in doc/guides/maintaining-c-ares.md.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
BethGriggs added a commit that referenced this pull request Nov 30, 2021
Notable changes:

- **deps**: upgrade npm to 8.1.2 (npm team)
  [#40643](#40643)
- **deps**: update c-ares to 1.18.1 (Richard Lau)
  [#40660](#40660)
- **doc**: add VoltrexMaster to collaborators (voltrexmaster)
  [#40566](#40566)
- **lib**: fix regular expression to detect \`/\` and \`\\\`
  (Francesco Trotta) [#40325](#40325)

PR-URL: #40974
BethGriggs added a commit that referenced this pull request Dec 1, 2021
Notable changes:

- **deps**: upgrade npm to 8.1.2 (npm team)
  [#40643](#40643)
- **deps**: update c-ares to 1.18.1 (Richard Lau)
  [#40660](#40660)
- **doc**: add VoltrexMaster to collaborators (voltrexmaster)
  [#40566](#40566)
- **lib**: fix regular expression to detect \`/\` and \`\\\`
  (Francesco Trotta) [#40325](#40325)

PR-URL: #40974
BethGriggs added a commit that referenced this pull request Dec 1, 2021
Notable changes:

- **deps**: upgrade npm to 8.1.2 (npm team)
  [#40643](#40643)
- **deps**: update c-ares to 1.18.1 (Richard Lau)
  [#40660](#40660)
- **doc**: add VoltrexMaster to collaborators (voltrexmaster)
  [#40566](#40566)
- **lib**: fix regular expression to detect \`/\` and \`\\\`
  (Francesco Trotta) [#40325](#40325)

PR-URL: #40974
richardlau added a commit that referenced this pull request Dec 9, 2021
Add a script to automate updating of the c-ares dependency and
accompanying maintenance guide.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
richardlau added a commit that referenced this pull request Dec 9, 2021
Updated as described in doc/guides/maintaining-c-ares.md.

PR-URL: #40660
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@richardlau richardlau mentioned this pull request Dec 13, 2021
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Notable changes:

- **deps**: upgrade npm to 8.1.2 (npm team)
  [nodejs#40643](nodejs#40643)
- **deps**: update c-ares to 1.18.1 (Richard Lau)
  [nodejs#40660](nodejs#40660)
- **doc**: add VoltrexMaster to collaborators (voltrexmaster)
  [nodejs#40566](nodejs#40566)
- **lib**: fix regular expression to detect \`/\` and \`\\\`
  (Francesco Trotta) [nodejs#40325](nodejs#40325)

PR-URL: nodejs#40974
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants