Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix install source path for openssl headers #1354

Closed
wants to merge 2 commits into from
Closed

fix install source path for openssl headers #1354

wants to merge 2 commits into from

Conversation

obastemur
Copy link
Contributor

This part is broken for a very long time. We noticed the problem while using jxcore native interface with embedded openssl. I've also sent a pull request to node.js repo. The problem may affect a native addon using builtin openssl.

This part is broken for a very long time. We noticed the problem while using jxcore native interface with embedded openssl. I've also sent a pull request to node.js repo. The problem may affect a native addon using builtin openssl.
@brendanashworth brendanashworth added the openssl Issues and PRs related to the OpenSSL dependency. label Apr 6, 2015
@rvagg
Copy link
Member

rvagg commented Apr 6, 2015

/cc @indutny @shigeki @bnoordhuis

thanks @obastemur

@shigeki
Copy link
Contributor

shigeki commented Apr 6, 2015

This has a problem because deps/openssl/conf/opensslconf.h is overwritten by deps/openssl/openssl/include/openssl/opensslconf.h. A fix to change the order to copy include files is needed such as

diff --git a/tools/install.py b/tools/install.py
index 094cadb..2d00cf0 100755
--- a/tools/install.py
+++ b/tools/install.py
@@ -173,8 +173,8 @@ def files(action):
     subdir_files('deps/uv/include', 'include/node/', action)

   if 'false' == variables.get('node_shared_openssl'):
-    action(['deps/openssl/config/opensslconf.h'], 'include/node/openssl/')
     subdir_files('deps/openssl/openssl/include/openssl', 'include/node/openssl/', action)
+    action(['deps/openssl/config/opensslconf.h'], 'include/node/openssl/')

   if 'false' == variables.get('node_shared_v8'):
     subdir_files('deps/v8/include', 'include/node/', action)

@obastemur Could you fix your patch?

@obastemur
Copy link
Contributor Author

@shigeki thanks. it's even better now.

@shigeki
Copy link
Contributor

shigeki commented Apr 6, 2015

Thanks. Just landed but I found a mistake in the commit message.
@rvagg I mistook the PR-URL number in 0e86685b8a3304193ab146a15ddbdb9559425f2f not 1350 but 1354. Can I rebase it and force push a new commit? Or stay this as it.

@brendanashworth
Copy link
Contributor

@shigeki though I'm not 100% sure and rvagg probably knows better, what I've heard people do is they amend the commit with the fix and force push. Unsure if that's right though.

shigeki pushed a commit that referenced this pull request Apr 6, 2015
This part is broken for a very long time. We noticed the problem while
using jxcore native interface with embedded openssl. I've also sent a
pull request to node.js repo. The problem may affect a native addon
using builtin openssl. `opensslconf.h` is overwritten with
`deps/openssl/conf/opensslconf.h`

PR-URL: #1354
Reviewed-By:  Shigeki Ohtsu <[email protected]>
@shigeki
Copy link
Contributor

shigeki commented Apr 6, 2015

@brendanashworth Thanks. I've just amend the previous commit and pushed a new one in ec7fbf2.

@shigeki shigeki closed this Apr 6, 2015
@rvagg
Copy link
Member

rvagg commented Apr 6, 2015

Probably worth talking this through on #1355, the general rule I've adopted is that a force-push to fix a mistake you've just made is OK as long as you're still at the HEAD, if something else has got in on top then that's just bad luck and you should move on.

I'm the wrong person to make up the rule here though, my git-fu is relatively week compared to others in here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants