Use modern way to set the C/CXX standard
ClosedPublic

Authored by vonreth on Oct 21 2019, 7:02 PM.

Details

Summary

This allows later modification of the selected standard.

Raise C from C89 to C90 as C89 is not supported by the CMAKE flag

https://cmake.org/cmake/help/v3.16/prop_tgt/C_STANDARD.html#prop_tgt:C_STANDARD

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vonreth created this revision.Oct 21 2019, 7:02 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptOct 21 2019, 7:02 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
vonreth requested review of this revision.Oct 21 2019, 7:02 PM
vonreth updated this revision to Diff 68475.Oct 21 2019, 7:04 PM

Change message

I would also use CMAKE_CXX_STANDARD_REQUIRED to make sure the compiler actually *can* c++11, otherwise the flag is only added when the compiler supports it. Should not make much difference nowadays but since it's in ECM...

I would also use CMAKE_CXX_STANDARD_REQUIRED to make sure the compiler actually *can* c++11, otherwise the flag is only added when the compiler supports it. Should not make much difference nowadays but since it's in ECM...

Good point.
I also tried to investigate the supported C standard by msvc, they say C99 is there but still incomplete.

vonreth edited the summary of this revision. (Show Details)Oct 21 2019, 7:07 PM
vonreth updated this revision to Diff 68478.Oct 21 2019, 7:08 PM

CMAKE_CXX_STANDARD_REQUIRED

dfaure requested changes to this revision.Oct 21 2019, 8:19 PM
dfaure added inline comments.
kde-modules/KDECompilerSettings.cmake
208

That's not how it works.
You want

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED TRUE)

Note however that these require CMake >= 3.1, while ECM says cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR). I do however thing that ECM *should* require 3.1.
I doubt it's been tested on 2.8.x in ages, and 3.1 is really a common requirement these days.

This revision now requires changes to proceed.Oct 21 2019, 8:19 PM
aacid added a subscriber: aacid.Oct 21 2019, 8:26 PM

If you want to increase cmake dependency, we should at least increase it to 3.5

https://repology.org/project/cmake/badges

Works for me.

I guess we should do that in a separate review?

Dunno, it's harder to justify in a standalone review :)
"Increase dependency, just because" ;)

vonreth updated this revision to Diff 68488.Oct 21 2019, 9:38 PM
  • Raise CMake requirements to 3.5
dfaure accepted this revision.Oct 22 2019, 7:00 AM
This revision is now accepted and ready to land.Oct 22 2019, 7:00 AM
cgiboudeaux retitled this revision from Use modern way to set the C/CXX standad to Use modern way to set the C/CXX standard.Oct 22 2019, 2:53 PM
cgiboudeaux edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.

I'm seeing build failures in several repositories seemingly caused by 6e3c794 (eg akonadi, kasync)

Building with clang9, before:
-std=c++0x -std=gnu++14 are set

After:
-std=gnu++11

and the build fails with c++14 features such as make_unique

Where did -std=gnu++14 come from? The old code above certainly didn't set it.

Maybe some projects were doing set(CMAKE_CXX_STANDARD 14) before including KDECompilerSettings? We could test the var here to avoid overwriting it...

Where did -std=gnu++14 come from? The old code above certainly didn't set it.

Maybe some projects were doing set(CMAKE_CXX_STANDARD 14) before including KDECompilerSettings? We could test the var here to avoid overwriting it...

Yes, the CXX standard is set before looking for ECM in both repositories.

Another issue caused by the new CMake 3.5 dependency, some tests fail:

59 - ecm_setup_version-old_simple (Failed)
60 - ecm_setup_version-old_soversion (Failed)
61 - ecm_setup_version-old_version_file (Failed)
62 - ecm_setup_version-old_version_file_abspath (Failed)
63 - ecm_setup_version-old_version_file_anynewer (Failed)
64 - ecm_setup_version-old_version_file_exact (Failed)
65 - ecm_setup_version-old_version_file_samemajor (Failed)
66 - ecm_setup_version-old_header (Failed)
67 - ecm_setup_version-old_header_abspath (Failed)

(+2 others not related to the recent changes)
I didn't look yet at the details. My guess is the CMake policy changes between 2.8.12 and 3.5

Another issue caused by the new CMake 3.5 dependency, some tests fail:

59 - ecm_setup_version-old_simple (Failed)
60 - ecm_setup_version-old_soversion (Failed)
61 - ecm_setup_version-old_version_file (Failed)
62 - ecm_setup_version-old_version_file_abspath (Failed)
63 - ecm_setup_version-old_version_file_anynewer (Failed)
64 - ecm_setup_version-old_version_file_exact (Failed)
65 - ecm_setup_version-old_version_file_samemajor (Failed)
66 - ecm_setup_version-old_header (Failed)
67 - ecm_setup_version-old_header_abspath (Failed)

(+2 others not related to the recent changes)
I didn't look yet at the details. My guess is the CMake policy changes between 2.8.12 and 3.5

Where are those tests running? I'm only aware of https://build.kde.org/job/Frameworks/job/extra-cmake-modules/

Where are those tests running? I'm only aware of https://build.kde.org/job/Frameworks/job/extra-cmake-modules/

The tests are currntly sadly skipped on CI, compare T11858

Someone(tm) needs to act on this, myself had no time yet.