Skip to content
This repository was archived by the owner on Jun 4, 2021. It is now read-only.

Make it compatible with Node 10.0 - 10.7 - #6

Merged
kolontsov merged 4 commits into
kolontsov:masterfrom
mildsunrise:compatibility-fixes
May 10, 2019
Merged

kolontsov merged 4 commits into
kolontsov:masterfrom
mildsunrise:compatibility-fixes

Conversation

@mildsunrise

Copy link
Copy Markdown
Contributor

Turns out it was easy :)

However, I had to patch binding.gyp as indicated here, otherwise the code used system OpenSSL headers instead of Node.JS version of OpenSSL.

@mildsunrise

Copy link
Copy Markdown
Contributor Author

Aaaand it turns out this is also happening in Travis. I'll take a look.

@mildsunrise

Copy link
Copy Markdown
Contributor Author

It's a bug in node-gyp that was fixed in node-gyp 3.7.0.
Node.JS 10.0 only has node-gyp 3.6.2.

@mildsunrise

Copy link
Copy Markdown
Contributor Author

I've ended up backporting the fix (it's two includes)

@kolontsov

kolontsov commented May 10, 2019 •

Copy link
Copy Markdown
Owner

Great work! it compiles and runs simple tests.

However I have concerns about &(*ssl_) (which makes code compatible with both SSL* and std::unique_ptr<SSL>), because operator* has undefined behaviour if get() returns nullptr, or if we're dereferencing NULL SSL* pointer. In our case it seems to work with null pointer because we use &*, but.. can we please switch to #ifdef or something like this, to use return ssl_ for <=10.7 and return ssl_.get() in other cases?

@mildsunrise

Copy link
Copy Markdown
Contributor Author

Done!

@kolontsov
kolontsov merged commit f0a8fc0 into kolontsov:master May 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants