metacall / core

MetaCall: The ultimate polyglot programming experience.
https://metacall.io
Apache License 2.0
1.57k stars 161 forks source link

Deprecation of write_compiler_detection_header in CMake #241

Closed viferga closed 1 year ago

viferga commented 2 years ago

🐛 Bug Report

Since CMake 3.20, the write_compiler_detection_header has been deprecated ( https://cmake.org/cmake/help/latest/module/WriteCompilerDetectionHeader.html ). My recommendation is to completely remove it. It is being used in a lot of parts of the CMake code and it generates a lot of unnecessary boilerplate.

All the <module>_features.h should be removed from the project, instead we can implement a library for full portability inside portability module, and remove all the ad-hoc #ifs that are present in the code.

For implementing the portability module, you can find some bases here:

We should remove: 1) This block from all CMakeFiles:

# Create feature detection header
# Compilers: https://cmake.org/cmake/help/v3.1/variable/CMAKE_LANG_COMPILER_ID.html#variable:CMAKE_%3CLANG%3E_COMPILER_ID
# Feature: https://cmake.org/cmake/help/v3.1/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html

# Check for availability of module; use pre-generated version if not found
if (WriterCompilerDetectionHeaderFound)
    write_compiler_detection_header(
        FILE ${feature_file}
        PREFIX ${target_upper}
        COMPILERS AppleClang Clang GNU MSVC
        FEATURES cxx_alignas cxx_alignof cxx_constexpr cxx_final cxx_noexcept cxx_nullptr cxx_sizeof_member cxx_thread_local
        VERSION 3.2
    )
else()
    file(
        COPY ${PROJECT_SOURCE_DIR}/codegeneration/${target}_features.h
        DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/include/${target}
        USE_SOURCE_PERMISSIONS
    )
endif()

2) Any include or reference to _features.h files. 3) Also remove this from the CMakeLists.txt in the root folder:

# This module is only available with CMake >=3.1, so check whether it could be found
include(WriteCompilerDetectionHeader OPTIONAL RESULT_VARIABLE WriterCompilerDetectionHeaderFound)

Then search for all ad-hoc #if referring to platform, etc.. in the code and substitute them accordingly to the definitions of portability module.

b2pacific commented 2 years ago

Hi @viferga ! I would like to work on this issue. Can you assign this to me? Also, could you give some context regarding this issue as to which files I need to modify? I would really appreciate that :)

Rajat1206 commented 2 years ago

Hi! I love this repo & the idea and I would like to work on this issue!

Samyak2 commented 2 years ago

I noticed this warning when building metacall today.

I see that it's used for these features cxx_alignas cxx_alignof cxx_constexpr cxx_final cxx_noexcept cxx_nullptr cxx_sizeof_member cxx_thread_local in the write_compiler_detection_header call present in the CMake lists.

Though, I couldn't find any usage of corresponding macros - _ALIGNAS, _ALIGNOF, _CONSTEXPR, _NOEXCEPT, _NULLPTR, _THREAD_LOCAL (I couldn't find cxx_sizeof_member in the CMake docs to find the corresponding macro name for it).

So if these macros aren't being used, I think we can safely remove the usage of write_compiler_detection_header and WriterCompilerDetectionHeader altogether. Am I missing something here?

I'll try remove it and see if build succeeds.

viferga commented 1 year ago

@Samyak2 you are right, it is completely unneeded. In my opinion it would be interesting to see what are the real dependencies of the Core itself or.. another option is to completely remove this and implement a crossplatform support on top of code and maybe a bit of custom cmake scripts.

This is usually superior and more effective, I have done this in the past.

viferga commented 1 year ago

First step towards the resolution of this issue is done: https://github.com/metacall/core/commit/5ea99d7dc3a0c5a5eacdf056bfff4ebb548a14fc

Next step is to standardize the compiler detection (and its features) from headers, and include architecture and OS detection. Here's the TODO: https://github.com/metacall/core/blob/5ea99d7dc3a0c5a5eacdf056bfff4ebb548a14fc/source/portability/include/portability/portability_compiler_detection.h#L24

viferga commented 1 year ago

Another thing we will need to do is to replace all the #if #else.. etc of the whole code, when there's debug detection, compiler detection or OS/Arch, and simplify it with portability headers.

viferga commented 1 year ago

Closing this issue as the rest to do is just an improvement, and the warning, which was the original issue, is solved