deps: add defines to nghttp3/ngtcp2 gyp configs#33942
Closed
danbev wants to merge 2 commits intonodejs:masterfrom
Closed
deps: add defines to nghttp3/ngtcp2 gyp configs#33942danbev wants to merge 2 commits intonodejs:masterfrom
danbev wants to merge 2 commits intonodejs:masterfrom
Conversation
danbev
added
experimental
Member
|
deps/ngtcp2/.ngtcp2.gyp.swp shouldn't have been checked in 😀. |
This commit adds two defines to ngtcp2, 'HAVE_ARPA_INET_H' and
'HAVE_NETINET_IN_H', and one define to nghttp3, 'HAVE_ARPA_INET_H' when
the operating system in use is linux.
The motivation for this is that currently, there are compiler warnings
generated for these libraries similar to the following:
../deps/ngtcp2/lib/ngtcp2_conv.c: In function ‘ngtcp2_put_uint16be’:
../deps/ngtcp2/lib/ngtcp2_conv.c:129:7: warning:
implicit declaration of function ‘htons’ [-Wimplicit-function-declaration]
129 | n = htons(n);
| ^~~~~
The inclusion of 'arpa/inet.h' is guarded by the 'HAVE_ARPA_INET_H'
macro (see deps/ngtcp2/lib/ngtcp2_conv.h).
These headers are checked by the ngtcp2 and nghttp3 builds using CMake's
CheckIncludeFile, and if they are available the above macros are defined.
I'm not sure if we need to have something similar to those checks or
ifit is alright to add these defines for certain operating systems.
Hopefully others will chime in and advice on how to handle this in the
best way.
danbev
force-pushed
the
deps-ngtcp2-nghttp3-compiler-warnings
branch
from
bf96b65 to
db858fc
Compare
Contributor
Author
|
@richardlau Thanks! |
jasnell
approved these changes
Jun 18, 2020
addaleax
approved these changes
Jun 19, 2020
Member
|
@danbev ... just checking, can this be moved out of draft yet? |
Contributor
Author
|
Yep, no problem (It’s a public holiday today but I can update it tomorrow,
but feel free to change it before that)
fre 19 juni 2020 kl. 16:27 skrev James M Snell <[email protected]>:
… @danbev <https://github.com/danbev> ... just checking, can this be moved
out of draft yet?
—
You are receiving this because you were mentioned
Reply to this email directly, view it on GitHub
<#33942 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADJRX35OR2AE26337PHBYTRXNYV3ANCNFSM4OBSJ5JA>
.
|
jasnell
pushed a commit
that referenced
this pull request
Jun 22, 2020
This commit adds two defines to ngtcp2, 'HAVE_ARPA_INET_H' and
'HAVE_NETINET_IN_H', and one define to nghttp3, 'HAVE_ARPA_INET_H' when
the operating system in use is linux.
The motivation for this is that currently, there are compiler warnings
generated for these libraries similar to the following:
../deps/ngtcp2/lib/ngtcp2_conv.c: In function ‘ngtcp2_put_uint16be’:
../deps/ngtcp2/lib/ngtcp2_conv.c:129:7: warning:
implicit declaration of function ‘htons’ [-Wimplicit-function-declaration]
129 | n = htons(n);
| ^~~~~
The inclusion of 'arpa/inet.h' is guarded by the 'HAVE_ARPA_INET_H'
macro (see deps/ngtcp2/lib/ngtcp2_conv.h).
These headers are checked by the ngtcp2 and nghttp3 builds using CMake's
CheckIncludeFile, and if they are available the above macros are defined.
I'm not sure if we need to have something similar to those checks or
ifit is alright to add these defines for certain operating systems.
Hopefully others will chime in and advice on how to handle this in the
best way.
PR-URL: #33942
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
jasnell
pushed a commit
that referenced
this pull request
Jun 22, 2020
PR-URL: #33942 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Member
|
Landed in a0cbd67 and a041723 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
This commit adds two defines to ngtcp2,
HAVE_ARPA_INET_HandHAVE_NETINET_IN_H, and one define to nghttp3,HAVE_ARPA_INET_Hwhenthe operating system in use is linux.
The motivation for this is that currently, there are compiler warnings
generated for these libraries similar to the following:
The inclusion of
arpa/inet.his guarded by theHAVE_ARPA_INET_Hmacro (see deps/ngtcp2/lib/ngtcp2_conv.h).
These headers are checked by the ngtcp2 and nghttp3 builds using CMake's
CheckIncludeFile, and if they are available the above macros are defined.
I'm not sure if we need to have something similar to those checks or
if it is alright to add these defines for certain operating systems.
Hopefully others will chime in and advice on how to handle this in the
best way.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes