Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Upgrade OpenSSL to 1.0.1m #9441

Closed
misterdjules opened this issue Mar 19, 2015 · 12 comments
Closed

Upgrade OpenSSL to 1.0.1m #9441

misterdjules opened this issue Mar 19, 2015 · 12 comments
Assignees
Milestone

Comments

@misterdjules
Copy link

See the announcement of the upcoming 1.0.1m release.

When the node v0.10.38 release that contains the OpenSSL upgrade is done, please merge into v0.12 and do the v0.12.1 release.

@misterdjules
Copy link
Author

@jasnell As discussed this morning, the plan for now is to first upgrade OpenSSL in v0.10, then to merge v0.10 into v0.12.1 branched off of v0.12.0, and finally to merge v0.12.1-release into v0.12.

@joyent/node-coreteam Please let us know if this is correct.

@jasnell
Copy link
Member

jasnell commented Mar 19, 2015

Sounds good.
On Mar 19, 2015 12:29 PM, "Julien Gilli" notifications@github.com wrote:

@jasnell https://github.com/jasnell As discussed this morning, the plan
for now is to first upgrade OpenSSL in v0.10, then to merge v0.10 into
v0.12.1 branched off of v0.12.0, and finally to merge v0.12.1-release into
v0.12.

@joyent/node-coreteam https://github.com/orgs/joyent/teams/node-coreteam
Please let us know if this is correct.


Reply to this email directly or view it on GitHub
#9441 (comment).

@jasnell
Copy link
Member

jasnell commented Mar 20, 2015

@joyent/node-coreteam ...

For review:
Patch against: v0.10.37-release - jasnell/node@8a917ca (my branch: https://github.com/jasnell/node/tree/v0.10.37-opensslm)

Patch against: v0.12.0-release - jasnell/node@5cc430f (my branch: https://github.com/jasnell/node/tree/v0.12.0-opensslm)

The patch followed the process documented in the io.js openssl-m update (nodejs/node#1206) by @shigeki

Builds on Mac OSx 10.10 and Ubuntu 14.10. Tests pass. Needs additional review and verification.

@shigeki
Copy link

shigeki commented Mar 20, 2015

I've just reviewed the v0.10.37-opensslm branch and verified there is no difference from my patch. It can work on Node-v0.10.x. LGTM.

@jasnell
Copy link
Member

jasnell commented Mar 20, 2015

@shigeki ... I appreciate your work. You made it significantly easier for
me 👍
On Mar 19, 2015 7:14 PM, "Shigeki Ohtsu" notifications@github.com wrote:

I've just reviewed the v0.10.37-opensslm branch and verified there is no
difference from my patch. It can work on Node-v0.10.x. LGTM.


Reply to this email directly or view it on GitHub
#9441 (comment).

@misterdjules
Copy link
Author

@shigeki @jasnell Looking at the changes, I think it would be better to make changes to deps/openssl/openssl.gyp instead of changes to the OpenSSL source itself to work around the symlinked headers. That makes us have to maintain one less floating patch.

I would also like to keep at least these commits:

as separate commits so that we can track them separately from the rest of the changes.

How does that sound?

@piscisaureus
Copy link

The problem with symlinked headers is that they're a pain on windows.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2015

See alternate PR: #9451

@shigeki
Copy link

shigeki commented Mar 21, 2015

@misterdjules It would be possible to move all headers to the directory out of OpenSSL source such as deps/openssl/config/openssl/include/ and change include_dirs in openssl.gyp. But I wonder if there are some addons to use openssl library which refers deps/openssl/openssl/include in their gyp and they are all affected.

@shigeki
Copy link

shigeki commented Mar 21, 2015

On openssl git repository, the include directory is empty and written in .gitignore. Makefile generates symlinks by using util/mklink.pl. But the distributed tar.gz file includes symlinks and it causes issues on Windows as pointed out by @piscisaureus . So we need to modify the original source so as to make the include directory be empty even when we change to use gyp.

@misterdjules
Copy link
Author

@piscisaureus @shigeki Thank you for the clarification, I didn't realize that the symlinks shim has been present for quite a while now.

@misterdjules
Copy link
Author

Fixed by c6e8a2c, 10717f6, 2b21c45, 63377ec, 15cdeb7, 1fc3fdf and 4b69c72.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants