move -Werror to CI only 📦

[huangminghuang]

Change Description

This PR remove -Werror compiler flag from the CMakeLists.txt and only use it for CI builds.

Change Type

Select ONE:

• Documentation

• Stability bug fix

• Other

• Other - special case

Testing Changes

Select ANY that apply:

• New Tests

• Existing Tests

• Test Framework

• CI System

• Other

Consensus Changes

• Consensus Changes

API Changes

• API Changes

Documentation Additions

• Documentation Additions

[spoonincode]

We probably want Werror on both CXX_FLAGS and C_FLAGS

[huangminghuang]

It doesn't work out of the box, see https://buildkite.com/EOSIO/eosio-build-unpinned/builds/9927#af15773f-5a88-4c25-984a-63586ce212e6.

There are a few options here:

1. just don't use CMAKE_C_FLAGS="-Werror"
2. convert "webassembly/runtimes/eos-vm-oc/gs_seg_helpers.c" into c++ and use extern "C"
3. remove target_compile_options( eos-vm INTERFACE "-Wno-register" ) in eos-vm/CMakeLists.txt and then use set_source_files_properties to set -Wno-register only on c++ files.

[spoonincode]

maybe you can do something like

target_compile_options(eos-vm INTERFACE $<$<COMPILE_LANGUAGE:CXX>:-Wno-register>)

It also doesn't seem all that bad to propagate -Wno-register to the root CMakeLists

[nksanthosh]

@spoonincode can you elaborate the goal here and why this is needed?
I am worried about the scenario “Local builds work and then it fails mysteriously on CI due to -Werror only on CI”

[spoonincode]

The problem is every compiler and version of the compiler has different warnings. And warnings are moved between Wall, Wextra, and Wpedantic between different versions. It's hard to predict what compiler users are going to use, and there are lot more combinations in the wild than our meager CI suite. As an example, I could no longer build eosio on my Linux box because both gcc 11 & clang 13 had warnings. clang 13 failed with,

libraries/abieos/include/eosio/name.hpp:21:14: error: definition of implicit copy assignment operator for 'name' is deprecated because it has a user-declared copy constructor [-Wdeprecated-copy,-Werror]
   constexpr name(const name&) = default;

This is an interesting warning, but it's not worth shooting down some poor user's build due to a benign warning like this.

I am worried about the scenario “Local builds work and then it fails mysteriously on CI due to -Werror only on CI”

This is a problem regardless (well, except for the "mysteriously" part, as long as one bothers to actually read the output) -- local developers are probably going to only build on one compiler locally before pushing, not the handful that are in CI.

That said, personally I would have gone with a cmake option (defaulted to off) that enabled Werror, so a developer can easily toggle and leave this on locally. I'm indifferent on that aspect though.

[spoonincode]

reason for this change?

[huangminghuang]

The -fpie would cause compiler warning in certain cases. See https://buildkite.com/EOSIO/eosio/builds/31652#96473ad1-aeb6-4b57-9827-0663654c7a86 which doesn’t have the change. Using CMAKE_POSITION_INDEPENDENT_CODE can make sure the cmake to use -fpic or -fpie correctly in all cases regardless building library or executable.

[spoonincode]

[2021-12-14T18:25:31Z] /usr/bin/ld: CMakeFiles/test_abieos_reflect.dir/src/reflect_test.cpp.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a PIE object; recompile with -fPIE
[2021-12-14T18:25:31Z] clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

That looks like a linker error though?

[huangminghuang]

The problem was cmake couldn't generate correct compile options. Here are the build.ninja snippets in both cases:

• with CMAKE_POSITION_INDEPENDENT_CODE=ON

build libraries/abieos/CMakeFiles/test_abieos_reflect.dir/src/reflect_test.cpp.o: CXX_COMPILER__test_abieos_reflect ../libraries/abieos/src/reflect_test.cpp || cmake_object_order_depends_target_test_abieos_reflect
  DEP_FILE = libraries/abieos/CMakeFiles/test_abieos_reflect.dir/src/reflect_test.cpp.o.d
  FLAGS = -Werror -Wall -Wsign-compare -Wrange-loop-analysis -Wno-unused-command-line-argument -Wall -Wno-invalid-partial-specialization -O3 -DNDEBUG -fPIE    -fdiagnostics-color=always -std=gnu++17
  INCLUDES = -I../libraries/abieos/include -isystem /usr/local/include/c++/v1 -isystem /usr/local/include -isystem /usr/include
  OBJECT_DIR = libraries/abieos/CMakeFiles/test_abieos_reflect.dir
  OBJECT_FILE_DIR = libraries/abieos/CMakeFiles/test_abieos_reflect.dir/src

• With original CMAKE_C_FLAGS_INIT, CMAKE_CXX_FLAGS_INIT and CMAKE_EXE_LINKER_FLAGS_INIT

build libraries/abieos/CMakeFiles/test_abieos_reflect.dir/src/reflect_test.cpp.o: CXX_COMPILER__test_abieos_reflect ../libraries/abieos/src/reflect_test.cpp || cmake_object_order_depends_target_test_abieos_reflect
  DEP_FILE = libraries/abieos/CMakeFiles/test_abieos_reflect.dir/src/reflect_test.cpp.o.d
  FLAGS = -Werror -Wall -Wsign-compare -Wrange-loop-analysis -Wno-unused-command-line-argument -Wall -Wno-invalid-partial-specialization -O3 -DNDEBUG    -fdiagnostics-color=always -std=gnu++17
  INCLUDES = -I../libraries/abieos/include -isystem /usr/local/include/c++/v1 -isystem /usr/local/include -isystem /usr/include
  OBJECT_DIR = libraries/abieos/CMakeFiles/test_abieos_reflect.dir
  OBJECT_FILE_DIR = libraries/abieos/CMakeFiles/test_abieos_reflect.dir/src
  TARGET_COMPILE_PDB = libraries/abieos/CMakeFiles/test_abieos_reflect.dir/
  TARGET_PDB = libraries/abieos/test_abieos_reflect.pdb

[spoonincode]

what's the reason for the CMP0077 change in abieos?

[huangminghuang]

huangminghuang 12 seconds ago
https://cmake.org/cmake/help/latest/policy/CMP0077.html
CMake version 3.22.1 warns when the policy is not set.

[spoonincode]

What are we doing that actually causes that warning though? It's not like cmake just always prints a warning now unless you dump that CMP0077 noise in the CMakeLists.

A really basic CMakeLists

cmake_minimum_required( VERSION 3.8 )
project(banana)

option(USE_PLANTAIN "intead of bananas, use plantains" OFF)

Then

$ cmake --version
cmake version 3.22.1
$ cmake -DUSE_PLANTAIN=On ..
(output removed; but no warnings)

[huangminghuang]

What are we doing that actually causes that warning though? It's not like cmake just always prints a warning now unless you dump that CMP0077 noise in the CMakeLists.

A really basic CMakeLists

cmake_minimum_required( VERSION 3.8 )
project(banana)

option(USE_PLANTAIN "intead of bananas, use plantains" OFF)

Then

$ cmake --version
cmake version 3.22.1
$ cmake -DUSE_PLANTAIN=On ..
(output removed; but no warnings)

Actually, it is only needed for 2.2.x and develop branches of eos and I just rebase all the changes from there. The problem stems from setting the option explicitly in parent project and then use add_subdirectory() to control the behavior the the subproject. In develop, we use ABIEOS_BUILD_SHARED_LIB to disable building the abieos shared library which causes the warning in the first place. ABIEOS_BUILD_SHARED_LIB is used to avoid building problem with cross compiling. Using EXCLUDE_FROM_ALL for abieos is not an option because we need to include the abieos unit tests. There's no CI pipeline for abieos at the moment. Even though we don't explicit set any option in this branch to trigger the warning, it can prevent warnings if someone want to set ABIEOS_NO_INT128 or ABIEOS_ONLY_LIBRARY explicitly in parent project which I admit the changes are very low. Personally, I prefer to keep the change to minimize the differences between eosio and eosio-2.1.x branches of abieos. I can remove the change if you really hate it.

[spoonincode]

Instead of

set(ABIEOS_BUILD_SHARED_LIB OFF)

Can use one of

option(ABIEOS_BUILD_SHARED_LIB "" OFF)

set(ABIEOS_BUILD_SHARED_LIB OFF CACHE BOOL "")

I think I like the option() variant more, but it appears we've used both methods in various spots so either is fine I guess.