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: make VC-WIN config generation deterministic #27543

Closed
wants to merge 1 commit into from

Conversation

jkunkee
Copy link
Contributor

@jkunkee jkunkee commented May 2, 2019

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.

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
@Trott
Copy link
Member

Trott commented May 3, 2019

@nodejs/crypto Should this be upstreamed? If not, is there some way to make sure these patches are applied to later OpenSSL updates?

@jkunkee
Copy link
Contributor Author

jkunkee commented May 3, 2019

These changes are to the system for integrating OpenSSL into the Node.js build system, not to OpenSSL itself, so they cannot be upstreamed. On the other hand, though, this means that this change will be automatically applied to all future OpenSSL revisions as they happen.

@Trott
Copy link
Member

Trott commented May 3, 2019

These changes are to the system for integrating OpenSSL into the Node.js build system, not to OpenSSL itself, so they cannot be upstreamed. On the other hand, though, this means that this change will be automatically applied to all future OpenSSL revisions as they happen.

Oh, I see.... deps/openssl is us, but deps/openssl/openssl is what gets replaced on upgrades. 👍

@sam-github sam-github requested a review from shigeki May 3, 2019 21:37
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@refack
Copy link
Contributor

refack commented May 3, 2019

So this is essentially a @nodejs/build-files change?

@refack refack added the build Issues and PRs related to build files or the CI. label May 3, 2019
@sam-github
Copy link
Contributor

Its a bug-fix to the deps/openssl/config/README.md process, the makefiles involved don't get used when node itself builds, they are used to regenerate the openssl conf when openssl is updated.

@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

The CI failures are strange since #27544 contains the same commit and the changes here are not part of the primary build system.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented May 13, 2019

The CI failures are strange since #27544 contains the same commit and the changes here are not part of the primary build system.

CI failures are likely unrelated test fragility. Re-running the failed jobs in CI....

Trott pushed a commit to Trott/io.js that referenced this pull request May 13, 2019
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.

PR-URL: nodejs#27543
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented May 13, 2019

Landed in 1aa99df

@Trott Trott closed this May 13, 2019
targos pushed a commit that referenced this pull request May 14, 2019
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.

PR-URL: #27543
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@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.

6 participants