build: export zlib symbols on Windows#7983
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
nodejs-github-bot added the build Author
ghost commented Aug 5, 2016
It would be really nice if this could land in time for Node.js 6.4.0 with the other build fixes. |
mscdex added zlib 
node.gyp Outdated
Member
bnoordhuis commented Aug 5, 2016
LGTM with style nit. CI: https://ci.nodejs.org/job/node-test-pull-request/3535/ To be clear, this only applies to non-ssl Windows builds? |
Author
ghost commented Aug 5, 2016
I added a dot in the end. This applies to both ssl and non-ssl:
The main point is to always export zlib symbols, with or without ssl being exported. |
Author
ghost commented Aug 5, 2016
A more to the point but less "technical" commit description could be "build: export zlib symbols on Windows". |
Author
ghost commented Aug 5, 2016
Rebased, added reviewer & changed title. It passed the tests. |
Member
jasnell commented Aug 5, 2016
LGTM |
Base the generated openssl.def on existing zlib.def. We cannot specify more than one DEF file per executable so we need to merge the two DEF files to expose both OpenSSL and Zlib functionality to addons. If OpenSSL is not used, link against zlib.def itself. PR-URL: #7983 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Author
ghost commented Aug 8, 2016
Could this get merged this week? I do rebase it frequently. I have one other pending commit that is tagged semver-minor and will get into 6.4.0: #7576 because of similar changes. So I guess this commit also should be tagged semver-minor and thus it needs to be committed in time for 6.4.0. |
Member
bnoordhuis commented Aug 8, 2016
CI is green so I'm okay with it getting merged. That particular configuration is not tested, of course, but it's unlikely to make matters worse than they currently are. /cc @nodejs/release - there is no land-on-v6.x label, how should I tag it for v6? |
Contributor
evanlucas commented Aug 8, 2016
If it cherry-picks cleanly, then you really don't need to do anything. |
addaleax added the semver-minor 
Member
addaleax commented Aug 8, 2016
One more CI: https://ci.nodejs.org/job/node-test-commit/4444/ |
Member
addaleax commented Aug 8, 2016
Landed in 359352c, thanks for the PR! |
addaleax pushed a commit that referenced this pull request Aug 8, 2016
Base the generated openssl.def on existing zlib.def. We cannot specify more than one DEF file per executable so we need to merge the two DEF files to expose both OpenSSL and Zlib functionality to addons. If OpenSSL is not used, link against zlib.def itself. PR-URL: #7983 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Author
ghost commented Aug 8, 2016
Thanks 🎉 |
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Base the generated openssl.def on existing zlib.def. We cannot specify more than one DEF file per executable so we need to merge the two DEF files to expose both OpenSSL and Zlib functionality to addons. If OpenSSL is not used, link against zlib.def itself. PR-URL: #7983 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
cjihrig added a commit that referenced this pull request Aug 11, 2016
Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070
cjihrig added a commit that referenced this pull request Aug 11, 2016
Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070
cjihrig added a commit that referenced this pull request Aug 15, 2016
Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070
cjihrig added a commit to cjihrig/node that referenced this pull request Aug 16, 2016
Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) nodejs#7983 and nodejs#7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) nodejs#7811 and nodejs#7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) nodejs#7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) nodejs#7942 * repl: The REPL now supports editor mode. (Prince J Wesley) nodejs#7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) nodejs#8013 Refs: nodejs#8020 PR-URL: nodejs#8070
4 tasks
Sign up for freeto 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.
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
build, addons
Description of change
Base the generated openssl.def on existing zlib.def. We cannot specify more than one DEF file per executable so we need to merge the two DEF files to expose both OpenSSL and Zlib functionality to addons.
If OpenSSL is not used, link against zlib.def itself.
@bnoordhuis knows about the mkssldef.py