BLD: Verify the ability to compile C++ sources before initiating the build by seiko2plus · Pull Request #20354 · numpy/numpy

seiko2plus · 2021-11-12T06:00:36Z

related to #20335

…build

rgommers · 2021-11-12T13:23:06Z

The one CI failure is:

cc1plus: warning: ‘-Werror=’ argument ‘-Werror=implicit-function-declaration’ is not valid for C++

rgommers · 2021-11-12T13:24:05Z

This seems like a good idea, thanks @seiko2plus! I'll do some local testing.

numpy/distutils/ccompiler.py

rgommers · 2021-11-13T11:04:45Z

The debug CI job still trips on the same flag:

C compiler: x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -g -Og -Wall -g -Og -fstack-protector-strong -Wformat -Werror=format-security -g -Og -fstack-protector-strong -Wformat -Werror=format-security -Wno-unused-result -Wsign-compare -g -Og -Wall -g -Og -fstack-protector-strong -Wformat -Werror=format-security -g -Og -fstack-protector-strong -Wformat -Werror=format-security -Werror=vla -Werror=nonnull -Werror=pointer-arith -Werror=implicit-function-declaration -Wlogical-op -Wno-sign-compare -Wno-maybe-uninitialized -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC

compile options: '-Inumpy/core/src/common -Inumpy/core/src -Inumpy/core -Inumpy/core/src/npymath -Inumpy/core/src/multiarray -Inumpy/core/src/umath -Inumpy/core/src/npysort -Inumpy/core/src/_simd -I/home/runner/work/numpy/numpy/builds/venv/include -I/usr/include/python3.8d -c'
x86_64-linux-gnu-gcc: _configtest.cxx
cc1plus: warning: ‘-Werror=’ argument ‘-Werror=implicit-function-declaration’ is not valid for C++

Looks like the config test attempts to use a C compiler on a .cxx test file here.

mattip · 2021-11-13T17:15:24Z

This is a cc1plus warning, so it is a c++ compiler and somehow it is getting a c-only option. NumPy defines the flag twice in the codebase, in numpy/testing/_private/extbuild.py and tools/travis-test.sh

The complete snippet

C compiler: x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare \
    -g -Og -Wall \
    -g -Og -fstack-protector-strong -Wformat -Werror=format-security \
    -g -Og -fstack-protector-strong -Wformat -Werror=format-security -Wno-unused-result -Wsign-compare 
    -g -Og -Wall \
    -g -Og -fstack-protector-strong -Wformat -Werror=format-security \
    -g -Og -fstack-protector-strong -Wformat -Werror=format-security \
    -Werror=vla -Werror=nonnull -Werror=pointer-arith -Werror=implicit-function-declaration \
    -Wlogical-op -Wno-sign-compare -Wno-maybe-uninitialized -Wdate-time \
    -D_FORTIFY_SOURCE=2 -fPIC

compile options: '-Inumpy/core/src/common -Inumpy/core/src -Inumpy/core -Inumpy/core/src/npymath \
    -Inumpy/core/src/multiarray -Inumpy/core/src/umath -Inumpy/core/src/npysort \
    -Inumpy/core/src/_simd -I/home/runner/work/numpy/numpy/builds/venv/include \
    -I/usr/include/python3.8d -c'
x86_64-linux-gnu-gcc: _configtest.cxx
cc1plus: warning: ‘-Werror=’ argument ‘-Werror=implicit-function-declaration’ is not valid for C++
x86_64-linux-gnu-g++ -pthread _configtest.o -o _configtest

rgommers · 2021-11-13T19:46:33Z

Looking at the other -Werror flags, that issue is clearly coming from travis-test.sh. That said, some extra protection for extbuild.py couldn't hurt. I pushed a commit with a proposed fix.

rgommers · 2021-11-13T19:55:22Z

Okay never mind, removed again. Should test more locally first.

seiko2plus · 2021-11-13T20:14:56Z

that issue is clearly coming from travis-test.sh

I have a workaround for this issue lay down in #19753, see:

but I don't think it's a good idea. however, manually filtering any invalid c++ flags isn't a better idea too. it seems that I need to move the invalid c++ filter to UnixCCompiler__compile because after tracing config_cmd.try_link() I found it doesn't call CCompiler_customize()

rgommers · 2021-11-13T20:19:31Z

numpy/distutils/ccompiler.py

+        ignore_c_flags = {
+            '-Werror=implicit-function-declaration', '-Wstrict-prototypes'
+        }
+        if hasattr(self, 'compiler_so'):


Just a guess, but the -Werror=implicit-function-declaration flag is present in both self.compiler_so and self.compiler. It's only removed from the former here. Maybe removing it from both is healthy; the code is too much of a spaghetti to be sure that only compiler_so will be used.

parm need_cxx only used by build_ext and build_clib that's why this filter has never been called, I took my changes back, kept the original one as-is inside compiler_cxx() now we have two C++ filters and we should to put them in one place.

I suggest just calling customize(need_cxx) inside cxx_compiler() which already clones the compiler instance, and we count only on method cxx_compiler() in the following code and the other one in build_clib to avoid duplicate initializing and unnecessary copying.

numpy/numpy/distutils/command/build_ext.py

Lines 256 to 265 in 20a2a70

if need_cxx_compiler:

self._cxx_compiler = new_compiler(compiler=compiler_type,

verbose=self.verbose,

dry_run=self.dry_run,

force=self.force)

compiler = self._cxx_compiler

compiler.customize(self.distribution, need_cxx=need_cxx_compiler)

compiler.customize_cmd(self)

compiler.show_customization()

self._cxx_compiler = compiler.cxx_compiler()

rgommers · 2021-11-13T20:34:32Z

it seems that I need to move the invalid c++ filter to UnixCCompiler__compile because after tracing config_cmd.try_link() I found it doesn't call CCompiler_customize()

Hmm, that's not good. IIRC that must always be done. try_link will end up calling finalize_options on the build command. I'm wondering is we can call .customize() there (like in customized_ccompiler in numpy/distutils/__init__.py).

Or implement try_link, calling customize() and then deferring to the distutils version of try_link.

seiko2plus · 2021-11-14T03:21:39Z

I was mistaken it was actually called customize but without need_cxx, I had to dig more to understand what is going on, so I ended up with an ugly workaround to avoid big changes.

seiko2plus · 2021-11-14T03:50:01Z

nothing better then the green color :)

rgommers

Nice workaround, LGTM. Thanks @seiko2plus!

seiko2plus added 36 - Build Build related PR C++ Related to introduction and use of C++ in the NumPy code base labels Nov 12, 2021

BLD: Verify the ability to compile C++ sources before initiating the …

d06f228

…build

seiko2plus force-pushed the test_cpp branch from a8d323b to d06f228 Compare November 12, 2021 06:11

seiko2plus added the 09 - Backport-Candidate PRs tagged should be backported label Nov 12, 2021

seiko2plus commented Nov 13, 2021

View reviewed changes

numpy/distutils/ccompiler.py Outdated Show resolved Hide resolved

seiko2plus force-pushed the test_cpp branch from e914f43 to bf8d56f Compare November 13, 2021 06:15

rgommers force-pushed the test_cpp branch from 612c473 to bf8d56f Compare November 13, 2021 19:55

rgommers reviewed Nov 13, 2021

View reviewed changes

DIST: Workaround ignore invalid C++ flags for config/try_link

b9173e5

seiko2plus force-pushed the test_cpp branch from bf8d56f to b9173e5 Compare November 14, 2021 03:16

rgommers approved these changes Nov 14, 2021

View reviewed changes

rgommers merged commit b8a0f33 into numpy:main Nov 14, 2021

rgommers added this to the 1.22.0 release milestone Nov 14, 2021

lithomas1 mentioned this pull request Nov 16, 2021

BUILD: use cibuildwheel to upload wheels #20262

Closed

3 tasks

seiko2plus mentioned this pull request Nov 18, 2021

BLD: Fix Macos Builds [wheel build] #20398

Merged

charris modified the milestones: 1.22.0 release, 1.21.5 release Nov 18, 2021

charris mentioned this pull request Nov 18, 2021

BLD: Fix Macos Builds [wheel build] #20402

Merged

charris mentioned this pull request Nov 25, 2021

BLD: Verify the ability to compile C++ sources before initiating the build #20464

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Nov 25, 2021

charris removed this from the 1.21.5 release milestone Nov 25, 2021

	if need_cxx_compiler:
	self._cxx_compiler = new_compiler(compiler=compiler_type,
	verbose=self.verbose,
	dry_run=self.dry_run,
	force=self.force)
	compiler = self._cxx_compiler
	compiler.customize(self.distribution, need_cxx=need_cxx_compiler)
	compiler.customize_cmd(self)
	compiler.show_customization()
	self._cxx_compiler = compiler.cxx_compiler()

Uh oh!

Conversation

seiko2plus commented Nov 12, 2021

Uh oh!

rgommers commented Nov 12, 2021

Uh oh!

rgommers commented Nov 12, 2021

Uh oh!

Uh oh!

rgommers commented Nov 13, 2021

Uh oh!

mattip commented Nov 13, 2021

Uh oh!

rgommers commented Nov 13, 2021

Uh oh!

rgommers commented Nov 13, 2021

Uh oh!

seiko2plus commented Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgommers Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

seiko2plus Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seiko2plus Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

rgommers commented Nov 13, 2021

Uh oh!

seiko2plus commented Nov 14, 2021

Uh oh!

seiko2plus commented Nov 14, 2021

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seiko2plus commented Nov 13, 2021 •

edited

Loading

seiko2plus Nov 14, 2021 •

edited

Loading