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

Simplify build #2

Merged
merged 1 commit into from
May 8, 2019
Merged

Conversation

mildsunrise
Copy link
Contributor

We can use --tarball instead of --nodedir, that way we don't have to extract the tar and it should work on Windows (but I think there's a bug with that in node-gyp).

Also check that node version is >= 10, not 10 or 11.

And simplify code in general (encoding not needed on https options, use stream.pipeline(...))

@kolontsov
Copy link
Owner

This is a great one; don't remember why I used 'nodedir' and not 'tarball' (may be I was relying on some C files originally? --tarball unpacks only *.h and *.gypi). And I agree with other changes.

But I don't want to enable node version 12 without adding it to Travis first. Could you check if it works? I expected that openssl-related things can change after 11, so I check for 10 & 11 specifically.

@kolontsov
Copy link
Owner

Also, nodejs was going to switch to openssl 1.1.1, so I planned to start using SSL_CTX_set_keylog_callback. Is it true for v12? But if current version still works in v12, we can live with existing code, of course.

@mildsunrise
Copy link
Contributor Author

I expected that openssl-related things can change after 11, so I check for 10 & 11 specifically.

Hmm, would you accept printing an "Unsupported / untested Node.JS version" warning instead of preventing installation?

But I don't want to enable node version 12 without adding it to Travis first. Could you check if it works?

I checked and it works with latest Node v12 :)
However v12 enables support for TLS 1.3, and if a TLS 1.3 connection is established it won't be logged properly (this can also happen in Node v11 if the user manually enables TLS 1.3).

@kolontsov
Copy link
Owner

Hmm, would you accept printing an "Unsupported / untested Node.JS version" warning instead of preventing installation?

Yes. I think it would be OK.

I checked and it works with latest Node v12 :)

Good news!

However v12 enables support for TLS 1.3, and if a TLS 1.3 connection is established it won't be logged properly (this can also happen in Node v11 if the user manually enables TLS 1.3).

Yes, TLS 1.3 is a separate issue, need to find time to implement proper support; PRs are welcome, of course :)

@mildsunrise
Copy link
Contributor Author

mildsunrise commented May 8, 2019

For reference, OpenSSL versions for each Node.JS release are:

v10.0.0 -> 1.1.0h
v10.9.0 -> 1.1.0i
v10.14.0 -> 1.1.0j
v10.16.0 -> 1.1.1b

v11.0.0 -> 1.1.0i
v11.3.0 -> 1.1.0j
v11.9.0 -> 1.1.1a
v11.12.0 -> 1.1.1b

v12.0.0 -> 1.1.1b

So SSL_CTX_set_keylog_callback should be available along with the session event and the rest of the API changes for TLSv1.3.

@kolontsov kolontsov merged commit 5186824 into kolontsov:master May 8, 2019
@kolontsov kolontsov mentioned this pull request May 8, 2019
@mildsunrise mildsunrise deleted the simplify-build branch August 19, 2019 07:48
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