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
Branch
cmake_c_standard
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17994
Build 18012: arc lint + arc unit
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
krop 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
krop edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
krop added a subscriber: krop.Oct 23 2019, 9:01 AM

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...

krop added a comment.Oct 23 2019, 9:17 AM

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.

krop added a comment.Oct 23 2019, 11:01 AM

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

In D24841#552600, @cgiboudeaux wrote:

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.