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

doc: update instructions for openssl updates #42353

Closed
wants to merge 9 commits into from

Conversation

mhdawson
Copy link
Member

Update to reflect additional PRs needed as some
branches now use OpenSSL 3.x

Signed-off-by: Michael Dawson [email protected]

Update to reflect additional PRs needed as some
branches now use OpenSSL 3.x

Signed-off-by: Michael Dawson <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 15, 2022
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

We could also optionally rename the "OpenSSL 3.0.0" heading later on to "OpenSSL 3.0.x" and maybe update to use a non-alpha example.

```console
$ make -C deps/openssl/config
$ git add deps/openssl/config/archs
$ git add deps/openssl/openssl
Copy link
Member Author

@mhdawson mhdawson Mar 16, 2022

Choose a reason for hiding this comment

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

@richard this matches what is in the commit comments below and looks like what we have done. It is probably also related to #42081 as git add deps/openssl/openssl adds all of the headers under openssl/openssl/include versus the selective ones that were added for OpenSSL 1.1.1.

I don't think we want to change right now as we get releases out, but worth looking to see if possibly we can revert to adding just those three, find out why more were needed and possibly ask @danbev when he gets back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I take that back since I guess the the examples were only showing the git adds that were necessary for a particular commit as opposed to what needs to be done in general. Therefore, it likely is not related to #42081 after all.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW AFAICT the big difference between OpenSSL 1.1.1 and 3.0.x in Node.js is the deps/openssl/config/archs -- there's many more files in OpenSSL 3.0.x under there (see description in #42081) and a preliminary glance suggested a lot of duplication between the archs.

@mhdawson
Copy link
Member Author

We could also optionally rename the "OpenSSL 3.0.0" heading later on to "OpenSSL 3.0.x" and maybe update to use a non-alpha example.

done

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. backport-requested-v16.x commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 17, 2022
@aduh95
Copy link
Contributor

aduh95 commented Mar 17, 2022

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42353
✔  Done loading data for nodejs/node/pull/42353
----------------------------------- PR info ------------------------------------
Title      doc: update instructions for openssl updates (#42353)
Author     Michael Dawson  (@mhdawson)
Branch     mhdawson:update-openssl-instructions -> nodejs:master
Labels     doc, backport-requested-v16.x, commit-queue-squash
Commits    9
 - doc: update instructions for openssl updates
 - Update doc/contributing/maintaining-openssl.md
 - Update doc/contributing/maintaining-openssl.md
 - Update doc/contributing/maintaining-openssl.md
 - squash address comments
 - squash:fixup
 - Update doc/contributing/maintaining-openssl.md
 - Update doc/contributing/maintaining-openssl.md
 - Update doc/contributing/maintaining-openssl.md
Committers 2
 - Michael Dawson 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/42353
Reviewed-By: Richard Lau 
Reviewed-By: Luigi Pinca 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42353
Reviewed-By: Richard Lau 
Reviewed-By: Luigi Pinca 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 15 Mar 2022 21:24:08 GMT
   ✔  Approvals: 3
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/42353#pullrequestreview-910851918
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/42353#pullrequestreview-912180819
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42353#pullrequestreview-912906182
   ✖  This PR needs to wait 11 more hours to land
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1997890318

mhdawson added a commit that referenced this pull request Mar 18, 2022
Update to reflect additional PRs needed as some
branches now use OpenSSL 3.x

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #42353
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@mhdawson
Copy link
Member Author

Landed in cc64c93

@mhdawson mhdawson closed this Mar 18, 2022
bengl pushed a commit that referenced this pull request Mar 21, 2022
Update to reflect additional PRs needed as some
branches now use OpenSSL 3.x

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #42353
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@bengl bengl mentioned this pull request Mar 21, 2022
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Update to reflect additional PRs needed as some
branches now use OpenSSL 3.x

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#42353
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants