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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

thangktran
Copy link
Contributor

@thangktran thangktran commented Feb 6, 2019 •
edited
Loading

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 nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 6, 2019
@thangktran thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from 7a69300 to 480681e Compare February 6, 2019 12:05
@thangktran
Copy link
Contributor Author

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

@danbev
Copy link
Contributor

danbev commented Feb 7, 2019

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

refack
refack suggested changes Feb 7, 2019
View reviewed changes
deps/openssl/openssl_common.gypi Outdated Show resolved Hide resolved
deps/openssl/openssl_common.gypi Outdated Show resolved Hide resolved
deps/openssl/openssl_common.gypi Outdated Show resolved Hide resolved
deps/openssl/openssl_common.gypi Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Feb 7, 2019

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 refack added the openssl Issues and PRs related to the OpenSSL dependency. label Feb 7, 2019
@refack
Copy link
Contributor

refack commented Feb 7, 2019

Windows fail is #25988

@thangktran thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from 480681e to b07957d Compare February 8, 2019 12:25
@thangktran
Copy link
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
Copy link
Contributor

refack commented Feb 8, 2019 •
edited
Loading

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

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

thefourtheye
thefourtheye reviewed Feb 11, 2019
View reviewed changes
configure.py Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Feb 11, 2019

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

@thangktran thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from b07957d to 4d792bb Compare February 11, 2019 21:03
richardlau
richardlau reviewed Feb 12, 2019
View reviewed changes
configure.py Outdated
# Fix based on work of @bnoordhuis from
# https://github.com/nodejs/node-v0.x-archive/issues/4186 .
o['variables']['clang'] = 1 if is_clang else 0
o['variables']['gcc_version'] = gcc_version[0] * 10 + gcc_version[1]
Copy link
Member

Choose a reason for hiding this comment

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

This isn't used anymore so should be removed.

Copy link
Contributor Author

@thangktran thangktran Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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 .

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to gcc_version.

Copy link
Contributor Author

@thangktran thangktran Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

@thangktran
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 thangktran force-pushed the thangktran/fix-clang-warnings-from-openssl branch from 4d792bb to 534ea4a Compare February 12, 2019 06:32
@thangktran
Copy link
Contributor Author

thangktran commented Feb 15, 2019 •
edited
Loading

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

@richardlau
Copy link
Member

richardlau commented Feb 15, 2019 •
edited
Loading

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

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

@richardlau
Copy link
Member

Landed in 128170f

@richardlau richardlau closed this Feb 15, 2019
richardlau pushed a commit that referenced this pull request Feb 15, 2019
@thangktran @richardlau
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 <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@thangktran 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
@thangktran @targos
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 <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@thangktran @rvagg
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 <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thefourtheye thefourtheye thefourtheye left review comments

@refack refack refack approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

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
@thangktran @danbev @refack @richardlau @jasnell @thefourtheye @nodejs-github-bot