Skip to content

deps: update OpenSSL 3.0.4+quic - #43589

Closed
RafaelGSS wants to merge 2 commits into
nodejs:mainfrom
RafaelGSS:deps/openssl-3-0-4
Closed

deps: update OpenSSL 3.0.4+quic#43589
RafaelGSS wants to merge 2 commits into
nodejs:mainfrom
RafaelGSS:deps/openssl-3-0-4

Conversation

@RafaelGSS

@RafaelGSSRafaelGSS commented Jun 27, 2022

Copy link
Copy Markdown
Member

Updated openssl dep to openssl-3.0.4p+quic using the maintenance guide.

Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-June/000228.html

@RafaelGSS
deps: upgrade openssl sources to quictls/openssl-3.0.4+quic
0cc5475
This updates all sources in deps/openssl/openssl by:
$ git clone git@github.com:quictls/openssl.git
$ cd openssl
$ git checkout openssl-3.0.4+quic
$ cd ../node/deps/openssl
$ rm -rf openssl
$ cp -R ../../../openssl openssl
$ rm -rf openssl/.git* openssl/.travis*
$ git add --all openssl
$ git commit openssl
@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@RafaelGSSRafaelGSS mentioned this pull request Jun 27, 2022
Closed
@nodejs-github-botnodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. labels Jun 27, 2022
@richardlaurichardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
mhdawson
mhdawson approved these changes Jun 27, 2022
View reviewed changes

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rubber stamp LGTM

@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2022

Copy link
Copy Markdown
Collaborator

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

@github-actionsgithub-actionsBot mentioned this pull request Jun 28, 2022
Open
32 tasks
@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator

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

@RafaelGSS

RafaelGSS commented Jun 28, 2022

Copy link
Copy Markdown
MemberAuthor

This keeps failing similar to #43536. @nodejs/build Honestly, I haven't found any valuable information why this is failing from the CI, any idea?

@richardlau

richardlau commented Jun 28, 2022

Copy link
Copy Markdown
Member

After investigation I think I've figured out what's going wrong. The platforms consistently failing are macOS and Windows and the commonality they have is that they are on case insensitive filesystems. If we look at one of the failing CI logs,
e.g. https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx11-x64/45854/console

12:54:44 Changes not staged for commit:
12:54:44 (use "git add <file>..." to update what will be committed)
12:54:44 (use "git restore <file>..." to discard changes in working directory)
12:54:44 modified: deps/openssl/config/archs/BSD-x86/asm/crypto/aes/aes-586.s
12:54:44 modified: deps/openssl/config/archs/BSD-x86/asm/crypto/aes/aesni-x86.s
...

and the corresponding source tree in this PR, https://github.com/RafaelGSS/node/tree/6c706e9b0f1fabbc20a9a9915cc9c02d9a191f8c/deps/openssl/config/archs/BSD-x86/asm/crypto/aes we can see that we have both:

This looks to have come about from quictls/openssl@6cfbb4b -- the intent was to rename the files from the lower case version to the upper case one. Probably a gap in our update instructions somewhere that means we've not removed the lower case versions.

@RafaelGSS

RafaelGSS commented Jun 28, 2022

Copy link
Copy Markdown
MemberAuthor

Thank you! I'll look at it.

@RafaelGSS
deps: update archs files for quictls/openssl-3.0.4+quic
f283982
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
$ make -C deps/openssl/config clean
$ make -C deps/openssl/config
$ git add deps/openssl/config/archs
$ git add deps/openssl/openssl
$ git commit
@RafaelGSS

RafaelGSS commented Jun 28, 2022

Copy link
Copy Markdown
MemberAuthor

I've run the make -C deps/openssl/config clean first. Let's see how it behaves. In case it works, I'll update the maintaining-openssl document.

@richardlau

richardlau commented Jun 28, 2022

Copy link
Copy Markdown
Member

https://github.com/nodejs/node/runs/7097656711?check_suite_focus=true#step:5:379

gyp: /home/runner/work/node/node/deps/openssl/config/archs/aix-gcc/no-asm/openssl.gypi not found (cwd: /home/runner/work/node/node) while reading includes of /home/runner/work/node/node/deps/openssl/openssl_no_asm.gypi while reading includes of /home/runner/work/node/node/deps/openssl/openssl.gyp while loading dependencies of /home/runner/work/node/node/node.gyp while trying to load /home/runner/work/node/node/node.gyp

ugh. Looks like we didn't update the gyp files when removing some of the archs in #42616 and didn't notice before because we never deleted the archs that were removed until make -C deps/openssl/config clean was run (they won't be put back by regenerating the config).

We'll need to update all of the .gypi files in deps/openssl to remove references to the architectures removed by #42616. @RafaelGSS Let me know if you be more comfortable with me doing that (would be tomorrow for me at the earliest now).

@github-actionsgithub-actionsBot mentioned this pull request Jun 29, 2022
Open
23 tasks
@F3n67uF3n67u mentioned this pull request Jun 29, 2022
Closed
@RafaelGSS

RafaelGSS commented Jun 30, 2022

Copy link
Copy Markdown
MemberAuthor

The cleanup of .s worked in #43603, but we're facing a new issue. Closing this one anyway.

@RafaelGSSRafaelGSS mentioned this pull request Jul 3, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@tniessentniessentniessen approved these changes
+1 more reviewer
@mhdawsonmhdawsonmhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dependenciesPull requests that update a dependency file.needs-ciPRs that need a full CI run.opensslIssues and PRs related to the OpenSSL dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@RafaelGSS@nodejs-github-bot@richardlau@jasnell@tniessen@mhdawson