search

libcheck / check

A unit testing framework for C
GNU Lesser General Public License v2.1
1.07k stars 210 forks source link

Refactor/cmake buildsystem configuration #314

Open jflopezfernandez opened 3 years ago

jflopezfernandez commented 3 years ago

Refactor CMake Buildsystem

This pull request does not add or remove any significant functionality. The only real difference is in the addition of compiler-, platform-, and build-type-specific compilation flags and definitions for additional diagnostics and stricter checks.

Please note: This pull request is very much a work in progress. The purpose of this pull request is to gauge the direction and style of the changes proposed here before making them permanent.

Summary of Changes

C Standard

Currently, the C standard is set using a global SET(CMAKE_C_STANDARD 99) command. This pull request modifies the declaration of the standard, removing this global declaration, and setting it via the SET_TARGET_PROPERTIES command. The C standard is now therefore not just a global variable, but a property of the target being built itself.

The C99 standard is modified analogously, and is still marked as required.

Compilation Flags

Default compilation flags were added for Clang, GCC, and MSVC to augment CMake's default flags. These flags are platform- and build-type specific.

Generator Expressions

The current configuration sets compilation flags procedurally using IF()...ENDIF() blocks. In this pull request, the buildsystem configuration is rewritten in a declarative style, with target properties being set using CMake generator expressions.

For example, the current configuration uses the following command to check if the current generator is MSVC. If it is, three compiler definitions are added to the global scope.

if(MSVC)
  add_definitions(-D_CRT_SECURE_NO_DEPRECATE)
  add_definitions(-D_CRT_SECURE_NO_WARNINGS)
  add_definitions(-D_CRT_NONSTDC_NO_WARNINGS)
endif(MSVC)

The refactored configuration instead sets these compiler definitions on the target, without polluting the global scope, and it does it via CMake generator expressions.

ADD_LIBRARY(check STATIC ...)

TARGET_COMPILE_DEFINITIONS(check
    PUBLIC
        $<$<BOOL:${MSVC}>:_CRT_SECURE_NO_DEPRECATE _CRT_SECURE_NO_WARNINGS _CRT_NONSTDC_NO_WARNINGS>
    ...
)

The nested generator expression above works as follows:

  1. If the current generator is Visual Studio, ${MSVC} is equal to 1.
  2. If ${MSVC} is equal to 1, $<BOOL:${MSVC}> evaluates to 1.
  3. If $<BOOL:${MSVC}> evaluates to 1, the following preprocessor macros are defined: _CRT_SECURE_NO_DEPRECATE, _CRT_SECURE_NO_WARNINGS, and _CRT_NONSTDC_NO_WARNINGS.

The usual -D prefix when defining preprocessor macros is discarded by CMake, and is not necessary.

pthreads FindModule

The current method of finding the pthreads library was replaced by the official CMake FindThreads module. The FindThreads module looks for thread libraries on the current system. It will return pthread-incompatible libraries as well (such as the native Win32 library), so there is a flag to specify whether a pthread-compatible library is required. The module has already been configured as necessary, and it has been tested successfully.

Testing

Both the release and the debug configurations generated by the configuration in this pull request were tested successfully on the following platforms and toolchains:

jflopezfernandez commented 3 years ago

Hey @brarcher Branden, I disabled the script-based tests while I finish getting the structure of the build system down, but I wanted to see what you thought of the changes so far. What I'm thinking is that rather than merging this branch, we can pick and choose the changes that seem most useful, and then refactor the current master branch one change at a time in a more organized way.

I think the primary benefit of this configuration style is that I think it's much easier to comprehend exactly what needs to be built and what uses what. For example, the creation of an object library so we don't have to build the library twice is something I'm thinking of knocking out as well. Or maybe cleaning up the libcompat build/link process.

What do you think?

mikkoi commented 3 years ago

Hi @jflopezfernandez

I did some refactoring in the CMake build about 1,5 years ago. I tried to make it create an object library but failed. It wasn't possible at the time. I ran into issues with target_link_libraries() and install(EXPORT ...). But maybe those problems have now been fixed in CMake. The build otherwise certainly can be improved. Thanks for all the care. :+1:

I deprecated the option CHECK_ENABLE_TESTS. :CMakeLists.txt:88. There is a comment in the source code. Please remove the option if you think it is time.

I wanted to use CMake's own system to run CppCheck quality checker but at the time there were problems with getting it work properly with build test server. The server didn't report the problems even when check reported errors. I did my work in the pull request https://github.com/libcheck/check/pull/222 .

jflopezfernandez commented 3 years ago

Hey @mikkoi, I saw that, that's actually where I got the idea from! Thank you for your comments, by the way, you have no idea how much they helped.

I saw the deprecation comment, too, but to be honest I'm not familiar enough with CMake's CTest features and functionality to know what's really going on there. All of the refactoring I did with the test executables was really just to set their compilation information directly via the TARGET_COMPILE_OPTIONS, TARGET_COMPILE_DEFINITIONS, etc. while using generator expressions.

I wanted to use CMake's own system to run CppCheck quality checker but at the time there were problems with getting it work properly with build test server.

I love the idea of using external tools like that, too. Doxygen is actually one of my favorite tools in the C/C++ toolchain, but when I started looking for a place to incorporate it using FindDoxygen, I got sidetracked until I eventually decided this change was too large and all-encompassing to be a good idea to do all at once.

Thanks for the link to the pull request, I'll go take a look at it so I can get a better idea of the direction you guys were already going in. Thanks again!

mikkoi commented 3 years ago

I saw the deprecation comment, too, but to be honest I'm not familiar enough with CMake's CTest features and functionality to know what's really going on there. All of the refactoring I did with the test executables was really just to set their compilation information directly via the TARGET_COMPILE_OPTIONS, TARGET_COMPILE_DEFINITIONS, etc. while using generator expressions.

It's really good to remove all add_definitions(). You are doing a good job. They and other CMake commands without target are really annoying and actually dangerous because they pollute the scope/namespace. The generator expressions are modern CMake.

It's a good idea to use the FindThreads module. But beware with CMake's Find** modules. Not all of them are updated and "modern". Some are really old and can have side effects. I discovered it firsthand when I was playing around with some of them. Most of them are okay, though.

Thank you.

brarcher commented 3 years ago

What I'm thinking is that rather than merging this branch, we can pick and choose the changes that seem most useful, and then refactor the current master branch one change at a time in a more organized way.

That sounds good. Here are my thoughts on the topics you mention.

C Standard

Sounds good

Compilation Flags

I don't get why platform-specific options are necessary. Are the defaults insufficient? Depending on platform specific configurations may result in a testing miss for other platforms that are not represented by our automated testing.

Generator Expressions

I'm not too familiar with how CMake works. I'll defer to @mikkoi's comment. I'm fine with such as change

pthreads FindModule

Sounds good

Testing

Testing a pull request manually on a few different platforms is OK. That may not scale generally, though. If the automated testing for PRs is insufficient, it may need to be improved.

mikkoi commented 3 years ago

A few comments on the changes as whole.

I understand what you mean by removing the rows

# Follow ISO C99 standard   
set(CMAKE_C_STANDARD 99)    
set(CMAKE_C_STANDARD_REQUIRED ON)   
set(CMAKE_C_EXTENSIONS ON)          # Use GNU extensions and POSIX standard

... and using SET_TARGET_PROPERTIES() together with the individual targets. But CMAKE_C_STANDARD is not a normal variable but a CMake variable and the default value for C_STANDARD property of targets. The variables CMAKE_C_STANDARD, CMAKE_C_STANDARD_REQUIRED, and CMAKE_C_EXTENSIONS enforce the requirements on all the compiled files in the project.

Style: We have used lowercase for CMake commands. Of course, in CMakeLists.txt files case doesn't matter but why change to capitals? CMake project recommends lowercase. Please see SO: tools-for-upper-lower-case-consistency-in-cmake-source.

jflopezfernandez commented 3 years ago

But CMAKE_C_STANDARD is not a normal variable but a CMake variable and the default value for C_STANDARD property of targets. The variables CMAKE_C_STANDARD, CMAKE_C_STANDARD_REQUIRED, and CMAKE_C_EXTENSIONS enforce the requirements on all the compiled files in the project.

Oh, I actually didn't know that these variables set the appropriate target properties, wow. I was actually thinking about how I didn't like that each target's C standard, requirement, and extensions property had to be individually set, because it meant that if we switched to C11 or C17 one day, you'd have to hunt down each target and manually propagate the requisite change.

Style: We have used lowercase for CMake commands. Of course, in CMakeLists.txt files case doesn't matter but why change to capitals?

You know, I could have sworn CMake recommended uppercasing all commands, but now that I'm thinking about it, I'm not sure why I thought this or where I got this impression. Maybe I'm confusing it with something else? Regardless, I actually thought I was helping by doing this, sorry about that; I'll be sure to adhere to the project style.

jflopezfernandez commented 3 years ago

I don't get why platform-specific options are necessary. Are the defaults insufficient? Depending on platform specific configurations may result in a testing miss for other platforms that are not represented by our automated testing.

I actually got the idea from the TODO list.

[ ] * use stricter CFLAGS for compiling Check

I think that the problem with adding default flags is that CMake is just supposed to generate the build files, after which you can just call make or whatever with the options you normally would. Having good default values for the debug configuration is a good shortcut though, I think, and I think it's unintrusive in the sense that no end users are meant to use the library in debug mode.

To answer your question specifically, however, the default compilation flags set by CMake (on Linux) are the following:

-- CMAKE_C_FLAGS_DEBUG: -g
-- CMAKE_C_FLAGS_DEBUG_INIT:  -g
-- CMAKE_C_FLAGS_DEBUG: -g
-- CMAKE_C_FLAGS_DEBUG_INIT:  -g
-- CMAKE_C_FLAGS_INIT:    
-- CMAKE_C_FLAGS_MINSIZEREL: -Os -DNDEBUG
-- CMAKE_C_FLAGS_MINSIZEREL_INIT:  -Os -DNDEBUG
-- CMAKE_C_FLAGS_RELEASE: -O3 -DNDEBUG
-- CMAKE_C_FLAGS_RELEASE_INIT:  -O3 -DNDEBUG
-- CMAKE_C_FLAGS_RELWITHDEBINFO: -O2 -g -DNDEBUG
-- CMAKE_C_FLAGS_RELWITHDEBINFO_INIT:  -O2 -g -DNDEBUG

I looked at the default flags on Windows yesterday, and they were pretty similar. For example, the debug configuration disables function inlining, but it doesn't significantly increase the output diagnostics verbosity, nor does it add all of the checks we would probably want.

I think that the benefit of providing compilation flags via the generator expressions is that a user simply has to specify the type of build they want, and then we can configure processor, system, and configuration specific options based on their selection, so that end-users don't need to pore over compiler documentation if they don't want to. If they do, though, we could implement an option to disable the inclusion of our default compiler flags, or even have no compilation flags as the default, but allowing users and devs to enable them with a single flag.

Again though, I was just going off of the TODO list, so I'm not sure if the concensus on the idea has evolved. I've actually been meaning to ask you how up to date that list is.

mikkoi commented 3 years ago

Style: We have used lowercase for CMake commands. Of course, in CMakeLists.txt files case doesn't matter but why change to capitals?

You know, I could have sworn CMake recommended uppercasing all commands, but now that I'm thinking about it, I'm not sure why I thought this or where I got this impression. Maybe I'm confusing it with something else? Regardless, I actually thought I was helping by doing this, sorry about that; I'll be sure to adhere to the project style.

Actually, you are right. CMake did recommend uppercasing earlier. But they changed their recommendation about 8 or so years ago. So there is a lot of contradictory tutorials around. CMake (program) itself does not enforce the style.There might be usable style linters but I have never used one.

mikkoi commented 3 years ago

I think that the benefit of providing compilation flags via the generator expressions is that a user simply has to specify the type of build they want, and then we can configure processor, system, and configuration specific options based on their selection, so that end-users don't need to pore over compiler documentation if they don't want to. If they do, though, we could implement an option to disable the inclusion of our default compiler flags, or even have no compilation flags as the default, but allowing users and devs to enable them with a single flag.

It's good to get more errors out of the build. That way future changes become more secure.

There is a possible alternative way to do this in another pull request. The PR also contains even more flags to set in GCC and Clang. The PR was not merged because I didn't manage to make the build server report errors in a proper way. And break build when errors occur.

brarcher commented 3 years ago

I don't get why platform-specific options are necessary. Are the defaults insufficient? Depending on platform specific configurations may result in a testing miss for other platforms that are not represented by our automated testing.

I actually got the idea from the TODO list.

Ah, I see. I'm fine with adding some additional flags to find warnings. However, you had a good point here:

I think that the problem with adding default flags is that CMake is just supposed to generate the build files, after which you can just call make or whatever with the options you normally would.

That is, the warning are important to us (so Check reduces the number of bugs it may have), so why don't we pass in the warnings we want in our PR test builds and not update the defaults themselves? The TODO was probably written at a time when there was no testing of releases except what one would do locally and manually, so adding new warnings helped the person creating the release. However, today every new PR is vetted on multiple platforms before acceptance, so when it is time to cut a release it should be trivial to do safely.

Thinking about it, I'm hesitant to update the default flags someone would use when building Check, beyond those that declare what variants of C we are using, etc. For example, what is the value of us specifying for Windows debug builds to "disable function inlining". It may be that setting such flags as defaults may cause issues later on.