build: export zlib symbols on Windows #7983
Closed
Conversation
This file contains 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
|
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
| @@ -369,9 +369,14 @@ | |||
| '-Wl,--no-whole-archive', | |||
| ], | |||
| }], | |||
| # openssl.def is based on zlib.def, zlib symbols | |||
| # are always exported | |||
There was a problem hiding this comment.
Can you punctuate the comment?
|
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? |
|
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. |
|
A more to the point but less "technical" commit description could be "build: export zlib symbols on Windows". |
ghost
changed the title
|
Rebased, added reviewer & changed title. It passed the tests. |
|
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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
|
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. |
|
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? |
|
If it cherry-picks cleanly, then you really don't need to do anything. |
addaleax
added
the
semver-minor
|
One more CI: https://ci.nodejs.org/job/node-test-commit/4444/ |
|
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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
|
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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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