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

deps: fix building with system c-ares on Linux #39739

Closed
wants to merge 2 commits into from
Closed

deps: fix building with system c-ares on Linux #39739

wants to merge 2 commits into from

Conversation

felixonmars
Copy link
Contributor

The change in #39724 breaks building with system c-ares (--shared-cares):

In file included from ../src/cares_wrap.cc:25:
../src/cares_wrap.h:25:11: fatal error: ares_nameser.h: No such file or directory
   25 | # include <ares_nameser.h>
      |           ^~~~~~~~~~~~~~~~

Since ares_nameser.h isn't available with a default system c-ares installation, let's add back the include check and use the old arpa/nameser.h routine instead.

Tested to build fine on Arch Linux with shared c-ares.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Aug 11, 2021
@felixonmars felixonmars changed the title Fix building with system c-ares on Linux deps: fix building with system c-ares on Linux Aug 11, 2021
@richardlau
Copy link
Member

Maybe we should take a copy of ares_nameser.h as in internal header in src for the platforms that don't have arpa/nameser.h instead of relying on the internal cares one?

@derrabus derrabus mentioned this pull request Aug 12, 2021
Closed
carlocab added a commit to derrabus/homebrew-core that referenced this pull request Aug 12, 2021
@carlocab
node: fix build with brewed c-ares
9086cde 
This applies the patch from nodejs/node#39739. This patch has been used
to ship Node 16.6.2 on Arch.
@carlocab
Copy link

This also fixes building with system c-ares on macOS. See Homebrew/homebrew-core#83106. Thanks for the patch, @felixonmars.

@richardlau richardlau linked an issue Aug 12, 2021 that may be closed by this pull request
v14.17.5 fails to build if configured with --shared-cares #39747
Closed
BrewTestBot pushed a commit to Homebrew/homebrew-core that referenced this pull request Aug 12, 2021
@derrabus @carlocab @BrewTestBot
node 16.6.2
f27857e 
* node 16.6.2
* node: fix build with brewed c-ares
  This applies the patch from nodejs/node#39739. This patch has been used
  to ship Node 16.6.2 on Arch.

Closes #83106.

Co-authored-by: Carlo Cabrera <[email protected]>
Signed-off-by: Carlo Cabrera <[email protected]>
Signed-off-by: BrewTestBot <[email protected]>
@jirutka
Copy link
Contributor

jirutka commented Aug 12, 2021

The same problem on Alpine Linux. c-ares does not install ares_nameser.h to /usr/include, it’s an internal header file, so probably shouldn’t be used by another projects.

@felixonmars
Copy link
Contributor Author

Maybe we should take a copy of ares_nameser.h as in internal header in src for the platforms that don't have arpa/nameser.h instead of relying on the internal cares one?

Should I just copy the file to src/? Perhaps some notes are needed for keeping this file updated on each c-ares bump in the future.

@richardlau
Copy link
Member

Maybe we should take a copy of ares_nameser.h as in internal header in src for the platforms that don't have arpa/nameser.h instead of relying on the internal cares one?

Should I just copy the file to src/? Perhaps some notes are needed for keeping this file updated on each c-ares bump in the future.

I think we do need notes or even a script for updating c-ares. Maybe do that in a separate PR and keep this one simple.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2021
@felixonmars
deps: fix building with system c-ares on Linux
aff98a5 
The change in nodejs#39724 breaks building with system c-ares
(`--shared-cares`):
```
In file included from ../src/cares_wrap.cc:25:
../src/cares_wrap.h:25:11: fatal error: ares_nameser.h: No such file or
directory
   25 | # include <ares_nameser.h>
      |           ^~~~~~~~~~~~~~~~
```

Since `ares_nameser.h` isn't available with a default system c-ares
installation, let's copy it as our private header here.

Tested to build fine on Arch Linux with shared c-ares.
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/39553/

@batrla
Copy link
Contributor

batrla commented Aug 13, 2021

Maybe we should take a copy of ares_nameser.h as in internal header in src for the platforms that don't have arpa/nameser.h instead of relying on the internal cares one?

Should I just copy the file to src/? Perhaps some notes are needed for keeping this file updated on each c-ares bump in the future.

Sorry, but in my opinion copying an internal header file from one project (c-ares) to use it in another project (Node.js) sounds like an ugly hack. To avoid special instructions on c-ares upgrade etc, the original project (c-ares) should instead make the header file (ares_nameser.h) public, promote it to an interface and start delivering it, right? In my eyes, it's an architectural issue.

@felixonmars felixonmars mentioned this pull request Aug 17, 2021
Closed
@felixonmars
Copy link
Contributor Author

Closing as c-ares/c-ares#417 is merged now. The header should be available in next c-ares release. No changes are needed here now.

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Aug 26, 2021
@andypost @jirutka
main/nodejs: security upgrade to 14.17.5
53d4c01 
- CVE-2021-3672
- CVE-2021-22931
- CVE-2021-22939

- https://nodejs.org/en/blog/release/v14.17.5/
- patch from nodejs/node#39739
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Aug 26, 2021
@andypost @jirutka
main/nodejs: security upgrade to 14.17.5
dd3221b 
- CVE-2021-3672
- CVE-2021-22931
- CVE-2021-22939

- https://nodejs.org/en/blog/release/v14.17.5/
- patch from nodejs/node#39739
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Aug 27, 2021
@andypost @jirutka
main/nodejs: security upgrade to 14.17.5
75fb18a 
- CVE-2021-3672
- CVE-2021-22931
- CVE-2021-22939

- https://nodejs.org/en/blog/release/v14.17.5/
- patch from nodejs/node#39739
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Sep 21, 2021
@bradleythughes
www/node14: Update 14.17.4 -> 14.17.5
d361a41 
https://nodejs.org/en/blog/release/v14.17.5/

This is a security release. See
https://nodejs.org/en/blog/vulnerability/aug-2021-security-releases/

Adopt the patch from nodejs/node#39739, since
Node.js does not build with a system installed c-ares without it.

Security:	b092bd4f-1b16-11ec-9d9d-0022489ad614
MFH:		2021Q3
Sponsored by:	Miles AS
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Sep 21, 2021
@bradleythughes
www/node: Update 16.6.1 -> 16.6.2
3d5b3fb 
https://nodejs.org/en/blog/release/v16.6.2/

This is a security release. See
https://nodejs.org/en/blog/vulnerability/aug-2021-security-releases/

Adopt the patch from nodejs/node#39739, since
Node.js does not build with a system installed c-ares without it.

Security:	b092bd4f-1b16-11ec-9d9d-0022489ad614
MFH:		2021Q3
Sponsored by:	Miles AS
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Sep 27, 2021
@bradleythughes
www/node14: Update 14.17.4 -> 14.17.5
69094f5 
https://nodejs.org/en/blog/release/v14.17.5/

This is a security release. See
https://nodejs.org/en/blog/vulnerability/aug-2021-security-releases/

Adopt the patch from nodejs/node#39739, since
Node.js does not build with a system installed c-ares without it.

Security:	b092bd4f-1b16-11ec-9d9d-0022489ad614
MFH:		2021Q3
Sponsored by:	Miles AS

(cherry picked from commit d361a41)
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Sep 27, 2021
@bradleythughes
www/node: Update 16.6.1 -> 16.6.2
647fb0e 
https://nodejs.org/en/blog/release/v16.6.2/

This is a security release. See
https://nodejs.org/en/blog/vulnerability/aug-2021-security-releases/

Adopt the patch from nodejs/node#39739, since
Node.js does not build with a system installed c-ares without it.

Security:	b092bd4f-1b16-11ec-9d9d-0022489ad614
MFH:		2021Q3
Sponsored by:	Miles AS

(cherry picked from commit 3d5b3fb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v14.17.5 fails to build if configured with --shared-cares
6 participants
@felixonmars @richardlau @carlocab @jirutka @nodejs-github-bot @batrla