build: cleanup CMake #319

compnerd · 2019-12-22T05:23:54Z

Some minor cleanups that I noticed which would improve things. This allows building on platforms which do not use libstdc++. There are still further refinements which can be made, but, this improves the state of things.

Rather than explicitly name the C++ runtime, use the `LINKER_LANGUAGE` property to use the driver to spell the C++ runtime appropriately.

Rather than use compiler specific flags to control the language standard, indicate to CMake the desired standard.

These flags are being applied to the *C* compiler, check the C compiler, not the C++ compiler.

This loosens the compiler identifier check to enable matching AppleClang which is the identifier for the Xcode compiler.

This hoists the common shared flags handling to the top-level CMakeLists from sub-layers. This prevents the duplication of the handling.

This is unnecessary, `/TP` is forced on all MSVC builds, no need to duplicate the flag for older versions.

Loosen the check to a match rather than equality check, this allows it to match AppleClang which is the identifier for the Apple vended clang compiler part of Xcode.

Use `add_compile_options` rather than modify `CMAKE_C_FLAGS`. The latter is meant to be only modified by the user, not the package developer.

This moves the CMAKE_C_FLAGS handling to the top-level and uses `add_compile_options` rather than modifying the user controlled flags.

These are global settings, hoist them to the top level.

Use a generator expression and hoist the flag handling for the debug build.

This is a global flag, hoist it to the top level and use `add_compile_options` rather than modify the user controlled flags.

This seemed to be attempting to set the linker not the linker flags for the profile configuration. This variable is not used, do not set it.

jgm · 2019-12-22T17:34:28Z

Many thanks. I barely know what I'm doing with cmake.
@nwellnhof do you have any objections to any of these changes?

nwellnhof · 2019-12-22T17:57:09Z

Looks good. I'm only curious about the claim that /TP is forced on all MSVC builds. All the tests pass, so this seems to be true. But why? Then we might also consider adding /TC when building with newer MSVC versions, although it's not strictly necessary.

compnerd · 2019-12-22T18:21:18Z

@jgm my pleasure :)

@nwellnhof, yeah, that is very subtle, and an accident that I caused. In fbe364a I hoisted the shared configuration between the api_test and src directory. The variant that I used was from the api_test directory which was subtly different: it always set /TP unlike the src version which limited it to the older releases only. If that is truly a difference that should be preserved, Im happy to adjust the stack to do so. However, after that commit, /TP is passed whenever using MSVC. At that point, setting it for the older release becomes redundant (though CMake should unique the flags). I figure that less build configuration is better.

jgm · 2019-12-24T18:35:35Z

Whoops, this PR caused problems with our automatic fuzzing setup: see the build log at

https://oss-fuzz-build-logs.storage.googleapis.com/log-b8018641-3053-4be0-80c1-f17c8d1ed4e8.txt

Step #4: -- Performing Test HAVE_FLAG_SANITIZE_ADDRESS - Success
Step #4: CMake Error at CMakeLists.txt:46 (add_compile_definitions):
Step #4:   Unknown CMake command "add_compile_definitions".
Step #4: 
Step #4: 
Step #4: -- Configuring incomplete, errors occurred!

Can you fix?

compnerd · 2019-12-24T23:18:54Z

Ugh, add_compile_definitions apparently was introduced in 3.12. I'll move that to add_compile_options as a workaround.

nwellnhof · 2019-12-28T14:35:46Z

However, after that commit, /TP is passed whenever using MSVC. At that point, setting it for the older release becomes redundant (though CMake should unique the flags). I figure that less build configuration is better.

Then I'd prefer to keep the version check which also serves as implicit documentation why the flag is needed.

waldyrious · 2019-12-28T15:44:28Z

I'd prefer to keep the version check which also serves as implicit documentation why the flag is needed.

Even better if a comment is added as explicit documentation.

compnerd added 15 commits December 21, 2019 20:42

build: inline a variable

0f4e228

build: use LINKER_LANGUAGE property for C++ runtime

fe0859e

Rather than explicitly name the C++ runtime, use the `LINKER_LANGUAGE` property to use the driver to spell the C++ runtime appropriately.

build: use CMake to control C standard

9c1399b

Rather than use compiler specific flags to control the language standard, indicate to CMake the desired standard.

build: use the correct variable

7ad8687

These flags are being applied to the *C* compiler, check the C compiler, not the C++ compiler.

build: loosen the compiler check

496a94d

This loosens the compiler identifier check to enable matching AppleClang which is the identifier for the Xcode compiler.

build: hoist shared flags to top-level CMakeLists

fbe364a

This hoists the common shared flags handling to the top-level CMakeLists from sub-layers. This prevents the duplication of the handling.

build: remove duplicated flags

1dff997

This is unnecessary, `/TP` is forced on all MSVC builds, no need to duplicate the flag for older versions.

build: loosen C compiler identifier check

cc7b060

Loosen the check to a match rather than equality check, this allows it to match AppleClang which is the identifier for the Apple vended clang compiler part of Xcode.

build: use add_compile_options

b1065ca

Use `add_compile_options` rather than modify `CMAKE_C_FLAGS`. The latter is meant to be only modified by the user, not the package developer.

build: hoist sanitizer flags to global state

18b587c

This moves the CMAKE_C_FLAGS handling to the top-level and uses `add_compile_options` rather than modifying the user controlled flags.

build: hoist -fvisibilty flags to top-level

db1616e

These are global settings, hoist them to the top level.

build: hoist the debug flag handling

911cbcd

Use a generator expression and hoist the flag handling for the debug build.

build: hoist the profile flag handling

b7ac560

This is a global flag, hoist it to the top level and use `add_compile_options` rather than modify the user controlled flags.

build: remove incorrect variable handling

24b7e83

This seemed to be attempting to set the linker not the linker flags for the profile configuration. This variable is not used, do not set it.

build: remove unused CMake includes

aba4282

jgm merged commit b6ffaca into commonmark:master Dec 22, 2019

compnerd deleted the cmake-cleanup branch December 22, 2019 18:21

mbrandonw mentioned this pull request Dec 22, 2019

Cmark fix pointfreeco/pointfreeco#536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: cleanup CMake #319

build: cleanup CMake #319

compnerd commented Dec 22, 2019

jgm commented Dec 22, 2019

nwellnhof commented Dec 22, 2019

compnerd commented Dec 22, 2019

jgm commented Dec 24, 2019

compnerd commented Dec 24, 2019

nwellnhof commented Dec 28, 2019

waldyrious commented Dec 28, 2019

build: cleanup CMake #319

build: cleanup CMake #319

Conversation

compnerd commented Dec 22, 2019

jgm commented Dec 22, 2019

nwellnhof commented Dec 22, 2019

compnerd commented Dec 22, 2019

jgm commented Dec 24, 2019

compnerd commented Dec 24, 2019

nwellnhof commented Dec 28, 2019

waldyrious commented Dec 28, 2019