http2: Allow using a shared nghttp2 library#15256
http2: Allow using a shared nghttp2 library#15256jer-gentoo wants to merge 2 commits intonodejs:masterfrom jer-gentoo:nghttp2_shared
Conversation
As nice as it is to bundle several libraries for builders' convenience, it also exposes builders to several kinds of security problems (until you release a new version with the bundled libraries updated) and it duplicates the number of versions of a library present on systems. For instance, with libcurl/curl installed and built against nghttp2, having a bundled (and older) version of libnghttp2 statically linked into /usr/bin/node duplicates the other version already present in /usr/lib*.
nodejs-github-bot
added
the
build
|
@nodejs/build PTAL |
|
Ping @nodejs/http2 @nodejs/build |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
It is beyond my area of expertise to know if this is correct. However, +1 we need this.
node.gyp
Outdated
| 'dependencies': [ | ||
| 'deps/nghttp2/nghttp2.gyp:nghttp2' | ||
| ] | ||
| }] |
There was a problem hiding this comment.
Is this necessary? It's covered by the conditional around line 712, right?
There was a problem hiding this comment.
I didn't write the conditionals for v8_enable_inspector==1, but I assumed that the author requires all bundled libraries in that case, so requiring the bundled nghttp2 as with the other bundled libraries seemed proper.
There was a problem hiding this comment.
Ah, sorry, I mean line 254 in node.gypi.
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
@jer-gentoo Can you rebase? Thanks.
node.gyp
Outdated
| 'dependencies': [ | ||
| 'deps/nghttp2/nghttp2.gyp:nghttp2' | ||
| ] | ||
| }] |
There was a problem hiding this comment.
Ah, sorry, I mean line 254 in node.gypi.
|
@bnoordhuis: Should be done now. |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Can you rebase onto master rather than merge master into your branch? Thanks!
| 'defines': [ | ||
| # We're using the nghttp2 static lib | ||
| 'NGHTTP2_STATICLIB' | ||
| ] |
There was a problem hiding this comment.
Can you remove the define? That's taken care of now that #15487 has been merged.
| 'NODE_WANT_INTERNALS=1', | ||
| # Warn when using deprecated V8 APIs. | ||
| 'V8_DEPRECATION_WARNINGS=1', | ||
| 'V8_DEPRECATION_WARNINGS=1' |
|
Sorry, I must have mixed up some local branches and pushed the wrong one. My git fu is weak.On 26 Sep 2017 3:31 p.m., Ben Noordhuis <[email protected]> wrote:@bnoordhuis requested changes on this pull request.
Can you rebase onto master rather than merge master into your branch? Thanks!
In node.gyp:
@@ -715,6 +714,15 @@
'deps/http_parser/http_parser.gyp:http_parser'
]
}],
+ [ 'node_shared_nghttp2=="false"', {
+ 'dependencies': [
+ 'deps/nghttp2/nghttp2.gyp:nghttp2'
+ ],
+ 'defines': [
+ # We're using the nghttp2 static lib
+ 'NGHTTP2_STATICLIB'
+ ]
Can you remove the define? That's taken care of now that #15487 has been merged.
In node.gyp:
@@ -300,7 +299,7 @@
'NODE_PLATFORM="<(OS)"',
'NODE_WANT_INTERNALS=1',
# Warn when using deprecated V8 APIs.
- 'V8_DEPRECATION_WARNINGS=1',
+ 'V8_DEPRECATION_WARNINGS=1'
Can you undo this change?
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@jer-gentoo I am not sure what you mean with pushing the wrong branch but to rebase you can do something like the following (depending on your git setup). |
|
@BridgeAR changed |
|
Or just |
|
Ping @jer-gentoo |
|
Ping @jer-gentoo. Do you plan to pursue this? A very similar PR has been opened in #16788. |
|
Superseded by #16788. |
8 participants
As nice as it is to bundle several libraries for builders' convenience,
it also exposes builders to several kinds of security problems (until
you release a new version with the bundled libraries updated) and it
duplicates the number of versions of a library present on systems. For
instance, with libcurl/curl installed and built against nghttp2, having
a bundled (and older) version of libnghttp2 statically linked into
/usr/bin/node duplicates the other version already present in /usr/lib*.