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

deps: update OpenSSL 3.0.4+quic #43589

Closed
wants to merge 2 commits into from
Closed

deps: update OpenSSL 3.0.4+quic #43589

wants to merge 2 commits into from

Conversation

RafaelGSS
Copy link
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
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@RafaelGSS RafaelGSS mentioned this pull request Jun 27, 2022
Closed
@nodejs-github-bot nodejs-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
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2022
@github-actions github-actions bot 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
Copy link
Member

@mhdawson mhdawson left a comment

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
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 28, 2022
Open
32 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@RafaelGSS
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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 force-pushed the deps/openssl-3-0-4 branch from 6c706e9 to f283982 Compare June 28, 2022 17:08
@RafaelGSS
Copy link
Member Author

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
Copy link
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-actions github-actions bot mentioned this pull request Jun 29, 2022
Open
23 tasks
@F3n67u F3n67u mentioned this pull request Jun 29, 2022
Closed
@RafaelGSS
Copy link
Member Author

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

@RafaelGSS RafaelGSS closed this Jun 30, 2022
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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.
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