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

fix install source path for openssl headers #14089

Closed
wants to merge 2 commits into from
Closed

fix install source path for openssl headers #14089

wants to merge 2 commits into from

Conversation

obastemur
Copy link

Seems node 0.12.x and node 0.10.x / jxcore use the same install script. This must be broken for a very long time. Moving the commit from jxcore/jxcore@fc1023f

Seems node 0.12.x and node 0.10.x / jxcore use the same install script. This must be broken for a very long time. Moving the commit from jxcore/jxcore@fc1023f
@obastemur
Copy link
Author

ping! this issue is breaking the native addons using builtin openssl.

@jasnell
Copy link
Member

jasnell commented Apr 6, 2015

LGTM /cc @joyent/node-coreteam

@jasnell jasnell added this to the 0.12.3 milestone Apr 6, 2015
@trevnorris
Copy link

LGTM

@misterdjules
Copy link

@obastemur Thank you!

LGTM, however it also applies to v0.10 and v0.12, so we may want to close this PR and submit another one against v0.10 that will get merged eventually into v0.12 and master.

Also, I'm not familiar with writing binary add-ons that require OpenSSL, but was this issue not raised before because it "only" impacts developers who use Node.js' OpenSSL headers without using node-gyp?

@obastemur
Copy link
Author

Also, I'm not familiar with writing binary add-ons that require OpenSSL, but was this issue not raised before because it "only" impacts developers who use Node.js' OpenSSL headers without using node-gyp?

Yes. IMHO this part should be removed entirely. openssl and the headers from other dependencies should be copied into include/deps folder (similar to node-gyp include packages). I have this item on my todo list. Once I have something proper for node 0.10, 0.12 I will be bringing it here at least for a discussion.

@misterdjules
Copy link

@obastemur Great thank you!

misterdjules pushed a commit to misterdjules/node that referenced this pull request Apr 23, 2015
PR: nodejs#14089
PR-URL: nodejs#14089
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
misterdjules pushed a commit to misterdjules/node that referenced this pull request Apr 23, 2015
PR: nodejs#14089
PR-URL: nodejs#14089
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
@misterdjules
Copy link

Landed in 9d19dfb and 4028669. These will get merged in v0.12 eventually (sooner than later). Thank you again @obastemur!

jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR: nodejs#14089
PR-URL: nodejs#14089
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR: nodejs#14089
PR-URL: nodejs#14089
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants