FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: fixed clang's warning when building openssl. by thangktran · Pull Request #25954 · nodejs/node · GitHub

/ node Public

build: fixed clang's warning when building openssl. - #25954

Closed
thangktran wants to merge 1 commit into
nodejs:masterfrom
thangktran:thangktran/fix-clang-warnings-from-openssl
ConversationCommits (1)ChecksFiles changed
Closed

build: fixed clang's warning when building openssl.#25954
thangktran wants to merge 1 commit into
nodejs:masterfrom
thangktran:thangktran/fix-clang-warnings-from-openssl

Conversation

thangktran commented Feb 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

clang doesn't seem to support 'Wno-old-style-declaration', this
is a work-around.

Fixes: #25550
Refs: nodejs/node-v0.x-archive#4186

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 6, 2019
thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from 7a69300 to 480681e Compare February 6, 2019 12:05

thangktran commented Feb 6, 2019

Copy link
Copy Markdown
Contributor Author

forced-pushed to fix the commit message according to guide-line.

danbev commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

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

refack suggested changes Feb 7, 2019
View reviewed changes
Comment thread deps/openssl/openssl_common.gypi Outdated
Comment thread deps/openssl/openssl_common.gypi Outdated
Comment thread deps/openssl/openssl_common.gypi Outdated
Comment thread deps/openssl/openssl_common.gypi Outdated
Comment thread configure.py Outdated

refack commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

Hello @thangktran and welcome. Thank you for your contribution 🥇
If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md

P.S. If you have any questions you can also feel free to contact me directly.

refack added the openssl Issues and PRs related to the OpenSSL dependency. label Feb 7, 2019

refack commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

Windows fail is #25988

thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from 480681e to b07957d Compare February 8, 2019 12:25

thangktran commented Feb 8, 2019

Copy link
Copy Markdown
Contributor Author

Hi @refack , thank you for your inputs.
I have just pushed the changes according to your suggestion.
One question: the ubuntu1604-arm64 test from node-test-commit-arm failed due to problem with

node/Makefile

Line 500 in 62942e9

.PHONY: test-ci

Was this error happened because the test processes were still running?

refack commented Feb 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Was this error happened because the test processes were still running?

It's a false positives, because there was manual testing being done on the CI worker at the same time. Should not repeat.

thangktran commented Feb 11, 2019

Copy link
Copy Markdown
Contributor Author

@refack The windows fail from #25988 is fixed.
Could you please run a new CI.
Thank you.

thefourtheye reviewed Feb 11, 2019
View reviewed changes
Comment thread configure.py Outdated

refack commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

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

thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from b07957d to 4d792bb Compare February 11, 2019 21:03
richardlau reviewed Feb 12, 2019
View reviewed changes
Comment thread configure.py Outdated

richardlau Feb 12, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This isn't used anymore so should be removed.

thangktran Feb 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@richardlau We still need 'clang' to enable -Wno-old-style-declaration for linux.
https://github.com/nodejs/node/blob/4d792bbe807d30b7c8f3af379f27f0747958faa2/deps/openssl/openssl_common.gypi#L66-L69
since clang doesn't seem to support this flag and causes #25550
I couldn't find this clang variable being set anywhere in the configure.py .

richardlau Feb 12, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I was referring to gcc_version.

thangktran Feb 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.
I found out that we can also use llvm_version instead of clang for the check.

build: fixed clang's warning when building openssl
534ea4a 
clang doesn't seem to support 'Wno-old-style-declaration', this
is a work-around.

Fixes: nodejs#25550
Refs: nodejs/node-v0.x-archive#4186
thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from 4d792bb to 534ea4a Compare February 12, 2019 06:32

thangktran commented Feb 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This PR is ready to land, could someone please take a look.
Thank you.

richardlau commented Feb 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/20784/ (✔️)

(Will land later today assuming this comes back non-red).

richardlau commented Feb 15, 2019

Copy link
Copy Markdown
Member

Landed in 128170f

richardlau closed this Feb 15, 2019
richardlau pushed a commit that referenced this pull request Feb 15, 2019
build: fixed clang's warning when building openssl
128170f 
clang doesn't seem to support 'Wno-old-style-declaration', this
is a work-around.

Fixes: #25550
Refs: nodejs/node-v0.x-archive#4186

PR-URL: #25954
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
thangktran deleted the thangktran/fix-clang-warnings-from-openssl branch February 15, 2019 12:44
targos pushed a commit that referenced this pull request Feb 15, 2019
build: fixed clang's warning when building openssl
40f4f42 
clang doesn't seem to support 'Wno-old-style-declaration', this
is a work-around.

Fixes: #25550
Refs: nodejs/node-v0.x-archive#4186

PR-URL: #25954
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
build: fixed clang's warning when building openssl
f408d78 
clang doesn't seem to support 'Wno-old-style-declaration', this
is a work-around.

Fixes: #25550
Refs: nodejs/node-v0.x-archive#4186

PR-URL: #25954
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
richardlau richardlau approved these changes
+3 more reviewers
refack refack approved these changes
thefourtheye thefourtheye left review comments
danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. 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.

Tons of unknown warning option '-Wno-old-style-declaration' when compiling openssl

7 participants

Back | FazBrowse Home | New Git URL