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

deps: regenerate OpenSSL configs with fixed tooling #27544

Closed
wants to merge 3 commits into from

Conversation

jkunkee
Copy link
Contributor

@jkunkee jkunkee commented May 2, 2019

This change uses previously checked-in fixes to the OpenSSL configuration
generation tooling to regenerate the broken configuration file.

Note that this is broken into two commits to highlight what actually changed
as opposed to what the tooling always changes.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

This change adds a clean target to the VC-WIN* Makefiles, then adjusts
the config generation script to call it before config file generation
as well as after. This prevents files from previous configurations from
causing make to incorrectly assume the files are up to date.
@nodejs-github-bot nodejs-github-bot added the openssl Issues and PRs related to the OpenSSL dependency. label May 2, 2019
@jkunkee
Copy link
Contributor Author

jkunkee commented May 2, 2019

This PR is dependent on 27543.

Also, note that this is part of the effort to bring ARM64 Windows support up to Experimental.

@Trott
Copy link
Member

Trott commented May 3, 2019

@nodejs/crypto (Same questions as the other PR.)

@sam-github
Copy link
Contributor

The 2nd and 3rd commit here are probably the output of running some tools, but they don't say what. I'm not clear as it is as to why they are split into two commits.

Could you add to the commit messages the command that you ran to generate the files being comitted? As an an example, see the previous commits to deps/openssl/archs, and also the instructions in deps/openssl/config/README.md

@refack refack added the build Issues and PRs related to build files or the CI. label May 3, 2019
jkunkee added 2 commits May 9, 2019 15:42
This change contains the results of running `make` in
`deps/openssl/config` (based on information in
deps/openssl/config/README.md) then reverting changes not in the
VC-WIN64-ARM directory.

This leverages a preceding change that fixes a cross-configuration file
reuse bug that only impacts VC-WIN64-ARM.
This change contains the results of running `make` in
`deps/openssl/config` (based on information in
deps/openssl/config/README.md) and not reverting anything.

This is not necessary, but it does indicate to the curious developer
that all architectures were automatically generated at the same time.
@jkunkee jkunkee force-pushed the fix-arm64-openssl-gen-output branch from 5e616cc to 9dd43eb Compare May 9, 2019 22:51
@jkunkee
Copy link
Contributor Author

jkunkee commented May 9, 2019

@sam-github, done.

The first commit is the same as the commit in #27543.

The second commit contains the only significant changes.

The third commit is not necessary, but it demonstrates that the preceding change does not have any unintended side effects. I would be happy to drop it.

Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

Let me confirm that this fix of deps/openssl/config/archs/VC-WIN64-ARM/no-asm/include/openssl/opensslconf.h is due to missing this clean targert in Makefile.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019
@nodejs-github-bot
Copy link
Collaborator

@jkunkee
Copy link
Contributor Author

jkunkee commented May 13, 2019

Would it be hard to get this and change integrated into v12 once it's in master?

@Trott
Copy link
Member

Trott commented May 13, 2019

Would it be hard to get this and change integrated into v12 once it's in master?

Non-breaking changes usually get cherry-picked into the latest stable release (currently v12.x) as a matter of course, so it should not be a problem, I don't think.

@Trott
Copy link
Member

Trott commented May 13, 2019

Let me confirm that this fix of deps/openssl/config/archs/VC-WIN64-ARM/no-asm/include/openssl/opensslconf.h is due to missing this clean targert in Makefile.

Hi, @shigeki! Can this land? Or should it wait a bit?

@shigeki
Copy link
Contributor

shigeki commented May 14, 2019

No problem to land this. I just wanted to have a confirmation.

Trott pushed a commit to Trott/io.js that referenced this pull request May 14, 2019
This change contains the results of running `make` in
`deps/openssl/config` (based on information in
deps/openssl/config/README.md) then reverting changes not in the
VC-WIN64-ARM directory.

This leverages a preceding change that fixes a cross-configuration file
reuse bug that only impacts VC-WIN64-ARM.

PR-URL: nodejs#27544
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request May 14, 2019
This change contains the results of running `make` in
`deps/openssl/config` (based on information in
deps/openssl/config/README.md) and not reverting anything.

This is not necessary, but it does indicate to the curious developer
that all architectures were automatically generated at the same time.

PR-URL: nodejs#27544
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented May 14, 2019

Landed in ac3b98c...38e11cc

@Trott Trott closed this May 14, 2019
targos pushed a commit that referenced this pull request May 15, 2019
This change contains the results of running `make` in
`deps/openssl/config` (based on information in
deps/openssl/config/README.md) then reverting changes not in the
VC-WIN64-ARM directory.

This leverages a preceding change that fixes a cross-configuration file
reuse bug that only impacts VC-WIN64-ARM.

PR-URL: #27544
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request May 15, 2019
This change contains the results of running `make` in
`deps/openssl/config` (based on information in
deps/openssl/config/README.md) and not reverting anything.

This is not necessary, but it does indicate to the curious developer
that all architectures were automatically generated at the same time.

PR-URL: #27544
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants