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

deps: use print() function on both Python 2 and 3 #24493

Closed
wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 19, 2018

@jasnell @refack This change was merged into upstream in openssl/openssl#7409

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the openssl Issues and PRs related to the OpenSSL dependency. label Nov 19, 2018
@jasnell
Copy link
Member

jasnell commented Nov 19, 2018

ping @nodejs/security @rvagg

@richardlau
Copy link
Member

Given the upstream PR was merged, it's possible we may get this in tomorrow's OpenSSL releases (we won't know for sure until the OpenSSL releases are out).

(Not blocking this if we want to float a patch btw).

@refack refack added blocked PRs that are blocked by other issues or PRs. python PRs and issues that require attention from people who are familiar with Python. labels Nov 19, 2018
@refack
Copy link
Contributor

refack commented Nov 19, 2018

(Not blocking this if we want to float a patch btw).

I'd wait with floating this since AFAIK fuzz/helper.py is not used in our build pipeline, so it's not blocking general python3 compatibility for node.

H/T to @cclauss who landed this upstream in openssl/openssl@83e4533

@shigeki
Copy link
Contributor

shigeki commented Nov 20, 2018

This is not include in OpenSSL-1.1.0 release for it is in the maintenance period.
I do not think it is a good idea to have it as a floating patch against OpenSSL-1.1.0 because it is not related to any crypto/TLS issues.
This is included in OpenSSL-1.1.1 and it is better to wait for the forthcoming OpenSSL-1.1.1a upgrade.

@cclauss
Copy link
Contributor Author

cclauss commented Nov 20, 2018

I do not see this change in the OpenSSL-1.1.1 tagged branch so waiting is warranted.

@shigeki
Copy link
Contributor

shigeki commented Nov 20, 2018

@cclauss You can see it in https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/fuzz/helper.py#L48.

@rvagg
Copy link
Member

rvagg commented Nov 21, 2018

I think I'd rather not merge this and just let it come in via the next OpenSSL releases since this is just for fuzzing, right? We don't do any fuzzing and I don't think we touch this path at all in any of our tools or infra. Or is someone doing fuzzing with the OpenSSL bundled with Node? (And if so .. why?)

@refack
Copy link
Contributor

refack commented Nov 21, 2018

So there's concensus to defer this.
@cclauss, as always, your effort is much appreciated. (And we get to learn and document the dark corners of our repo).

@refack refack closed this Nov 21, 2018
@shigeki
Copy link
Contributor

shigeki commented Nov 21, 2018

Note that we need to update OpenSSL-1.1.1 before Sep. 11, 2019 and this fix will be applied to Node-v10 above before EOL of python2 on Jan. 1, 2020.

@cclauss
Copy link
Contributor Author

cclauss commented Nov 21, 2018

This sounds like a good plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. openssl Issues and PRs related to the OpenSSL dependency. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants