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

fix install source path for openssl headers#14089

Closed
obastemur wants to merge 2 commits intonodejs:masterfrom
obastemur:patch-1
Closed

fix install source path for openssl headers#14089
obastemur wants to merge 2 commits intonodejs:masterfrom
obastemur:patch-1

Conversation

@obastemur
Copy link
Copy Markdown

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
Copy Markdown
Author

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

@jasnell
Copy link
Copy Markdown
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
Copy Markdown

LGTM

@misterdjules
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

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