Skip to content

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Jul 11, 2022

This was forgotten in: #43545
Ref: nodejs/nodejs.org#4713

This commit adds a new command line option named
'--openssl-shared-config' intended to allow reverting to the old OpenSSL
configuration behavior where Node.js would use the configuration section
name (called appname in OpenSSL) 'openssl_conf' which could potentially
be used my other applications..

PR-URL: #43124
Refs: #40366
Reviewed-By: James M Snell [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Rafael Gonzaga [email protected]
Reviewed-By: Beth Griggs [email protected]

@RafaelGSS RafaelGSS requested a review from danbev July 11, 2022 18:54
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v16.x labels Jul 11, 2022
This commit adds a new command line option named
'--openssl-shared-config' intended to allow reverting to the old OpenSSL
configuration behavior where Node.js would use the configuration section
name (called appname in OpenSSL) 'openssl_conf' which could potentially
be used my other applications..

PR-URL: nodejs#43124
Refs: nodejs#40366
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@RafaelGSS RafaelGSS force-pushed the backport-43124-v16-cli branch from 7399ebe to ecdead2 Compare July 12, 2022 16:44
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Jul 18, 2022

I'm sorry, there is now a conflict with other commits that landed on v16.x-staging. Could you please update the backport?
The original commit now lands cleanly but there's a compiler error:

../../src/node.cc:1102:35: error: no member named 'openssl_shared_config' in 'node::PerProcessOptions'; did you mean 'openssl_config'?
    if (per_process::cli_options->openssl_shared_config) {
                                  ^~~~~~~~~~~~~~~~~~~~~
                                  openssl_config
../../src/node_options.h:246:15: note: 'openssl_config' declared here
  std::string openssl_config;
              ^
1 error generated

I noted that in the original change, the flag only exists if node is compiled with OpenSSL 3 but that's not the case with this PR. Is that intended?

@RafaelGSS
Copy link
Member Author

I noted that in the original change, the flag only exists if node is compiled with OpenSSL 3 but that's not the case with this PR. Is that intended?

Yes, it was added this way because the initial idea was to support v17.x > only, but we need the revert flag in other release lines as well.

Thanks for notifying me, I'll rebase

@RafaelGSS
Copy link
Member Author

Closing in favor of: #43892. Sorry, due to many conflicts was better to create a new fresh PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants