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.

Sign up for GitHub

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

Jump to bottom

src: unify crypto constant setup #27077

Closed
wants to merge 1 commit into from
Closed

src: unify crypto constant setup #27077

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Apr 3, 2019 •
edited
Loading

DefineCryptoConstants() sets constants from OpenSSL into
crypto.constants, for crypto and tls. DefineOpenSSLConstants() did
exactly the same. Unify them.

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 c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 3, 2019
@sam-github sam-github mentioned this pull request Apr 3, 2019
Closed
2 tasks
addaleax
addaleax reviewed Apr 3, 2019
View reviewed changes
src/node_constants.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22187/

@sam-github
src: unify crypto constant setup
7b87ffb 
DefineCryptoConstants() sets constants from OpenSSL into
`crypto.constants`, for crypto and tls.  DefineOpenSSLConstants() did
exactly the same.  Unify them.
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22199/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22203/

@sam-github
Copy link
Contributor Author 

11:47:56   Generating code
11:47:56 c:\workspace\node-compile-windows\src\node_main.cc : fatal error C1382: the PCH file 'c:\workspace\node-compile-windows\release\obj\node\node.pch' has been rebuilt since 'c:\workspace\node-compile-windows\release\obj\node\node_main.obj' was generated. Please rebuild this object [c:\workspace\node-compile-windows\node.vcxproj]
11:47:56 LINK : fatal error LNK1257: code generation failed [c:\workspace\node-compile-windows\node.vcxproj]

That's odd, the build failed on https://ci.nodejs.org/job/node-compile-windows/25743/label=win-vs2017-x86/console

I resumed.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2019
@sam-github
Copy link
Contributor Author

@nodejs/build #27077 (comment) has happened twice in a row now, see the end of https://ci.nodejs.org/job/node-compile-windows/25746/label=win-vs2017-x86/consoleText, is this a known problem?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22227/

@nodejs nodejs deleted a comment from nodejs-github-bot Apr 5, 2019
@Trott
Copy link
Member

Trott commented Apr 5, 2019

@nodejs/build #27077 (comment) has happened twice in a row now, see the end of https://ci.nodejs.org/job/node-compile-windows/25746/label=win-vs2017-x86/consoleText, is this a known problem?

FWIW, both times here appear to be test-rackspace-win2012r2-x64-10. Build history for that host shows 5 consecutive failures that include those two failures, but then the host recovered. I have no idea if it self-healed or if someone intervened. I also don't know if this is a known issue.

@sam-github
Copy link
Contributor Author

@Trott Yes, I see the same thing. Its not clear if something was changed to fix it, but this just passed on Windows, too.

Yay?

@sam-github
Copy link
Contributor Author

Landed in a3f30a4

@sam-github sam-github closed this Apr 8, 2019
@sam-github sam-github deleted the unify-crypto-constants branch April 8, 2019 16:47
sam-github added a commit that referenced this pull request Apr 8, 2019
@sam-github
src: unify crypto constant setup
a3f30a4 
DefineCryptoConstants() sets constants from OpenSSL into
`crypto.constants`, for crypto and tls.  DefineOpenSSLConstants() did
exactly the same.  Unify them.

PR-URL: #27077
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sam-github @nodejs-github-bot @Trott @danbev @jasnell @addaleax @cjihrig @richardlau @BridgeAR