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

Update cares_wrap.cc #38572

Closed
wants to merge 4 commits into from
Closed

Update cares_wrap.cc #38572

wants to merge 4 commits into from

Conversation

VlkrS
Copy link

@VlkrS VlkrS commented May 6, 2021

OpenBSD does not define AI_ALL. Proposal is to treat the same as AI_4VMAPPED.

guybedford and others added 4 commits April 8, 2021 16:39
@guybedford
[v12.x] deps: update to cjs-module-lexer@1.1.1
411a41e 
PR-URL: #38003
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@phillipj @targos
build: label PRs with GitHub Action instead of nodejs-github-bot
c592529 
Main goal of using a GitHub Action for labelling PRs has been to move
the mapping between files changed -> label into a configuration file
local to the nodejs/node repository.

Previously any changes to that mapping meant having to grasp the
nodejs/github-bot project, open a PR with the neccessary changes, get
approval from its maintainers before those changes finally got pushed
to production.

The logic involved in using the file paths / label configuration and
resolving the labels to be applied, has been moved into a custom GitHub
Action project: nodejs/node-pr-labeler.

Aside from removing the external dependency the nodejs-github-bot is in
practise, it also reduces the bar for contributors since the resulting
project is a lot smaller and less complex than nodejs/github-bot.

PR-URL: #38301
Fixes: nodejs/github-bot#294
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
build: fix label-pr workflow
41fdf01 
PR-URL: #38399
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@VlkrS
Update cares_wrap.cc
1d2cbd1 
OpenBSD does not define AI_ALL.
@github-actions github-actions 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. v12.x labels May 6, 2021
addaleax
addaleax reviewed May 6, 2021
View reviewed changes
src/cares_wrap.cc
Comment on lines 52 to 54
#if defined(__OpenBSD__)
# define AI_ALL 0
# define AI_V4MAPPED 0
Copy link
Member

Choose a reason for hiding this comment

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

Obviously, this was this way before, but I'm wondering why we're not doing

Suggested change
#if defined(__OpenBSD__)
# define AI_ALL 0
# define AI_V4MAPPED 0
// OpenBSD does not define these
#ifndef AI_ALL
#define AI_ALL 0
#endif
#ifndef AI_V4MAPPED
#define AI_V4MAPPED 0

which seems just a bit more future-proof (because OpenBSD might add support at some point) and flexible (because what if there's other OSes that also don't have these)?

Copy link
Author

Choose a reason for hiding this comment

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

Looks good to me. From what I can tell, that change should apply to 14.x as well, no?

Copy link
Member

Choose a reason for hiding this comment

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

@VlkrS Err… yeah, I just noticed that this PR was opened against v12.x. Is there a reason for that? Typically changes are being made against the master branch first and then backported, unless there’s a reason not to.

Copy link
Author

Choose a reason for hiding this comment

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

The current version in OpenBSD ports is 12.16.1 and I was trying to upgrade that (I'm just a user and not involved in the project), so my concern was mainly to get a more recent 12.x version to build on account of the numerous security patches.

I can confirm that with the current patches in the port and this fix, I get a clean build and as far as I can tell a working node environment on OpenBSD, but I have not even looked into building any later release branch, let alone MASTER yet, so I thought it'd be best to stick to the branch I've actually tested :-)

jasnell
jasnell approved these changes May 7, 2021
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with @addaleax's suggested change

@addaleax addaleax mentioned this pull request May 13, 2021
Closed
addaleax added a commit to addaleax/node that referenced this pull request May 13, 2021
@addaleax
src: update cares_wrap OpenBSD defines
10a4b78 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
nodejs#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: nodejs#38572
@addaleax
Copy link
Member

PR for master: #38670

addaleax added a commit that referenced this pull request May 17, 2021
@addaleax
src: update cares_wrap OpenBSD defines
75340f3 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 18, 2021
@addaleax @targos
src: update cares_wrap OpenBSD defines
524a9d6 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
@addaleax @targos
src: update cares_wrap OpenBSD defines
ab58700 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
@addaleax @targos
src: update cares_wrap OpenBSD defines
b22b270 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
@addaleax @targos
src: update cares_wrap OpenBSD defines
bcf539c 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
@addaleax @targos
src: update cares_wrap OpenBSD defines
8809ef9 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 23, 2021
@addaleax @richardlau
src: update cares_wrap OpenBSD defines
2ce08cf 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@richardlau
Copy link
Member

I cherry-picked the relevant bits from #38670 to v12.x-staging in 2ce08cf.

@richardlau richardlau closed this Jul 23, 2021
guybedford pushed a commit that referenced this pull request Jul 26, 2021
@addaleax @guybedford
src: update cares_wrap OpenBSD defines
4a85a4b 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 26, 2021
@addaleax @richardlau
src: update cares_wrap OpenBSD defines
30ce0e6 
Move the `#define`s back into `cares_wrap.cc`, as they are part of
the implementation, not the declarations used in `cares_wrap.h`,
and apply the suggestion from
#38572 (comment)
to make the defines a bit more generic and not check for OpenBSD
specifically.

Refs: #38572

PR-URL: #38670
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@VlkrS VlkrS deleted the patch-1 branch September 3, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell 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.
8 participants
@VlkrS @addaleax @richardlau @jasnell @Trott @guybedford @phillipj @targos