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

build: add -Werror=undefined-inline to clang builds #23961

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 29, 2018

Hopefully the start of a trend to move more cases to -Werror

Refs: #23954
Refs: #23910
Refs: #23880

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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 Oct 29, 2018
@refack refack added c++ Issues and PRs that require attention from people who are familiar with C++. meta Issues and PRs related to the general management of the project. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 29, 2018
@refack refack requested a review from targos October 29, 2018 18:46
@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

@refack refack changed the title build: add -Werror="undefined-inline" to clang builds build: add -Werror=undefined-inline to clang builds Oct 29, 2018
@refack refack added the blocked PRs that are blocked by other issues or PRs. label Oct 29, 2018
@targos
Copy link
Member

targos commented Oct 29, 2018

I guess we should block on #23954?

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

Fails as expected on macOS (with clang)

15:06:27   /usr/local/bin/ccache c++ -o /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj.target/node_lib/src/inspector/worker_inspector.o ../src/inspector/worker_inspector.cc '-D_DARWIN_USE_64_BIT_INODE=1' '-DNODE_ARCH="x64"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' '-DHAVE_INSPECTOR=1' '-DHAVE_DTRACE=1' '-D__POSIX__' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DNODE_PLATFORM="darwin"' '-DHAVE_OPENSSL=1' '-DUCONFIG_NO_SERVICE=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DNGHTTP2_STATICLIB' -I../src -I/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj/gen -I/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj/gen/include -I/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj/gen/src -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes -I../deps/openssl/openssl/include  -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -Wall -Wendif-labels -W -Wno-unused-parameter -Werror=undefined-inline -std=gnu++1y -stdlib=libc++ -fno-rtti -fno-exceptions -fno-strict-aliasing -MMD -MF /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/.deps//Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj.target/node_lib/src/inspector/worker_inspector.o.d.raw   -c
15:06:28 In file included from ../src/inspector/worker_inspector.cc:3:
15:06:28 In file included from ../src/inspector/main_thread_interface.h:8:
15:06:28 In file included from ../src/env.h:27:
15:06:28 In file included from ../src/aliased_buffer.h:7:
15:06:28 ../src/util.h:65:11: error: inline function 'node::Calloc<unsigned char>' is not defined [-Werror,-Wundefined-inline]
15:06:28 inline T* Calloc(size_t n);
15:06:28           ^
15:06:28 ../src/aliased_buffer.h:41:15: note: used here
15:06:28     buffer_ = Calloc<NativeT>(count);
15:06:28               ^
15:06:28 1 error generated.
15:06:28 make[2]: *** [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj.target/node_lib/src/inspector/worker_inspector.o] Error 1

Waiting for #23954

@targos
Copy link
Member

targos commented Oct 29, 2018

Why didn't it fail on FreeBSD?

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

Why didn't it fail on FreeBSD?

Probably because the clang GYP variable is not set. I'll fix that in a separate PR.

@refack refack self-assigned this Oct 30, 2018
@refack refack removed the blocked PRs that are blocked by other issues or PRs. label Oct 30, 2018
@refack
Copy link
Contributor Author

refack commented Oct 30, 2018

#23954 landed so unblocked.
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18236/

@bnoordhuis
Copy link
Member

@refack Did you forget to rebase? It's still failing with the same build error.

@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

Did you forget to rebase?

pushed without force, and didn't look at results...

new resume: https://ci.nodejs.org/job/node-test-pull-request/18260/

PR-URL: nodejs#23961
Refs: nodejs#23954
Refs: nodejs#23910
Refs: nodejs#23880
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

Two more hours, so lets CI again: https://ci.nodejs.org/job/node-test-pull-request/18262/

@refack refack merged commit c515e5c into nodejs:master Oct 31, 2018
@refack refack deleted the Werror-undefined-inline branch October 31, 2018 23:40
targos pushed a commit that referenced this pull request Nov 2, 2018
PR-URL: #23961
Refs: #23954
Refs: #23910
Refs: #23880
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack refack removed their assignment Nov 6, 2018
@MylesBorins
Copy link
Contributor

opted to land on 10.x but not 8.x

MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
PR-URL: #23961
Refs: #23954
Refs: #23910
Refs: #23880
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23961
Refs: #23954
Refs: #23910
Refs: #23880
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23961
Refs: #23954
Refs: #23910
Refs: #23880
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
PR-URL: #23961
Refs: #23954
Refs: #23910
Refs: #23880
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@barracuda156
Copy link

Not only it breaks the build with GCC, but it gets pulled out from somewhere even after being patched out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants