Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmake: Check for C++-17 and conditionally enable it #188

Closed
wants to merge 1 commit into from
Closed

cmake: Check for C++-17 and conditionally enable it #188

wants to merge 1 commit into from

Conversation

philn
Copy link

@philn philn commented May 6, 2020

In GCC __cplusplus will be 201703 only if the -std=c++17 option is passed to the
compiler. Without this option the filesystem_utils.cpp won't compile under GCC
9.3.0, giving the following error:

src/common/filesystem_utils.cpp:74:10: fatal error: experimental/filesystem: No
such file or directory

@philn
cmake: Check for C++-17 and conditionally enable it
9e00104 
In GCC __cplusplus will be 201703 only if the -std=c++17 option is passed to the
compiler. Without this option the filesystem_utils.cpp won't compile under GCC
9.3.0, giving the following error:

src/common/filesystem_utils.cpp:74:10: fatal error: experimental/filesystem: No
such file or directory
@CLAassistant
Copy link

CLAassistant commented May 6, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@rpavlik
Copy link
Contributor

rpavlik commented May 7, 2020

Ah, so they have removed the experimental headers in 9.3.0? In the older versions of GCC (and libstdc++) I've used, the C++14 setting that's on now is enough to get the experimental filesystem support. I'll look closer at this soon.

@rpavlik rpavlik self-requested a review May 7, 2020 15:17
@philn
Copy link
Author

philn commented May 7, 2020

From what I can see here, the filesystem header is indeed no longer in experimental/.

@rpavlik rpavlik mentioned this pull request May 13, 2020
Merged
@rpavlik
Copy link
Contributor

rpavlik commented May 13, 2020

Thanks for doing this! Can you test and see if the branch in #192 works for you? I ended up making a fairly dramatic change, and actually checking to see if we could build using experimental/filesystem and filesystem (and filesystem with c++17 enabled), as well as then checking if we needed stdc++fs (since I can build c++17 mode here on Debian Buster with 8.3, but still need stdc++fs) to make it compile.

rpavlik
rpavlik requested changes May 13, 2020
View reviewed changes
src/CMakeLists.txt
include(CheckCXXCompilerFlag)

if("${CMAKE_MAJOR_VERSION}" EQUAL 3 AND "${CMAKE_MINOR_VERSION}" GREATER 7)
check_cxx_compiler_flag(-std=c++17 HAVE_FLAG_STD_CXX17)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to do this that I've found is to search CMAKE_CXX_COMPILE_FEATURES for cxx_std_17:

list(FIND CMAKE_CXX_COMPILE_FEATURES cxx_std_17 cxx17_index)
if(NOT ${cxx17_index} EQUAL -1)
    set(CMAKE_CXX_STANDARD 17)
endif()

This currently will not behave as desired on MSVC.

This change could be rebased on top of #192 - it's no longer needed to make it build on gcc 9.3.0 (I hope!) but it would still be worth considering an "opportunistic" upgrade to C++17.

src/loader/CMakeLists.txt

if(HAVE_FLAG_STD_CXX17)
target_link_libraries(
openxr_loader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this could be split into two target_link_libraries calls - one with just m, and one with stdc++fs. However, m isn't apparently actually needed, and at least on my gcc 8.3.0, I need stdc++fs even though I have c++17 support. I'd suggest just dropping this part of the patch when rebasing on #192 since that handles it in a more complete way.

@rpavlik
Copy link
Contributor

rpavlik commented May 28, 2020

Closing because #192 was merged and was reported to fix the issue.

@rpavlik rpavlik closed this May 28, 2020
@philn philn deleted the gcc-9.3.0 branch May 29, 2020 07:01
rpavlik added a commit that referenced this pull request May 29, 2020
@rpavlik
OpenXR SDK 1.0.9 (2020-05-29)
51cade1 
-   Registry
    -   Add an author ID, and reserve a vendor extension for Huawei.
        (OpenXR-Docs/#46)
    -   Reserve vendor extensions for future LunarG overlay and input
        focus functionality. (internal MR 1720)
    -   Reserve vendor extensions for Microsoft. (internal MR 1723)
    -   Add XR_EXT_hand_tracking multi-vendor extension. (internal MR
        1554, internal issue 1266, internal issue 1267, internal issue
        1268, internal issue 1269)
    -   Add XR_HUAWEI_controller_interaction vendor extension.
        (OpenXR-Docs/#47)
    -   Add XR_MNDX_egl_enable provisional vendor extension.
        (OpenXR-Docs/#48)
    -   Add XR_MSFT_spatial_graph_bridge vendor extension. (internal MR
        1730)
    -   Add XR_MSFT_secondary_view_configuration and
        XR_MSFT_first_person_observer vendor extensions. (internal MR
        1731)
    -   Add XR_MSFT_hand_mesh_tracking vendor extension. (internal MR
        1736)
    -   Fix missing space in XML definition of
        XrSpatialAnchorCreateInfoMSFT. (internal MR 1742, internal issue
        1351, OpenXR-SDK-Source/#187)
    -   Update a number of contacts for author/vendor tags. (internal MR
        1788, internal issue 1326)
-   SDK
    -   Replaced usage of the _DEBUG macro with NDEBUG. (internal MR
        1756)
    -   Allow disabling of std::filesystem usage via CMake, and detect
        if it’s available and what its requirements are.
        (OpenXR-SDK-Source/#192, OpenXR-SDK-Source/#188)
    -   CI: Modifications to Azure DevOps build pipeline. Now builds UWP
        loader DLLs in addition to Win32 loader DLLs. No longer builds
        static loader libraries due to linkability concerns. Re-arranged
        release artifact zip to distinguish architecture from 32-bit or
        64-bit.
    -   Loader: Replace global static initializers with functions that
        return static locals. With this change, code that includes
        OpenXR doesn’t have to page in this code and initialize these
        during startup. (OpenXR-SDK-Source/#173)
    -   Loader: Unload runtime when xrCreateInstance fails. (internal MR
        1778)
    -   Loader: Add “info”-level debug messages listing all the places
        that we look for the OpenXR active runtime manifest.
        (OpenXR-SDK-Source/#190)
    -   Validation Layer: Fix crash in dereferencing a nullptr optional
        array handle when the count > 0. (internal MR 1709,
        OpenXR-SDK-Source/#161, internal issue 1322)
    -   Validation Layer: Fix static analysis error and possible loss of
        validation error. (internal MR 1715, OpenXR-SDK-Source/#160,
        internal issue 1321)
    -   Validation Layer: Simplify some generated code, and minor
        performance improvements. (OpenXR-SDK-Source/#176)
    -   API Dump Layer: Fix crash in dereferencing a nullptr while
        constructing a std::string. (internal MR 1712,
        OpenXR-SDK-Source/#162, internal issue 1323)
    -   hello_xr: Fix releasing a swapchain image with the incorrect
        image layout. (internal MR 1755)
    -   hello_xr: Prefer VK_LAYER_KHRONOS_validation over
        VK_LAYER_LUNARG_standard_validation when available. (internal MR
        1755)
    -   hello_xr: Optimizations to D3D12 plugin to avoid GPU pipeline
        stall. (internal MR 1770) (OpenXR-SDK-Source/#175)
    -   hello_xr: Fix build with Vulkan headers 1.2.136.
        (OpenXR-SDK-Source/#181, OpenXR-SDK-Source/#180, internal issue
        1347)
    -   hello_xr: Fix build with Visual Studio 16.6.
        (OpenXR-SDK-Source/#186, OpenXR-SDK-Source/#184)
rhabacker pushed a commit to rhabacker/OpenXR-SDK-Source that referenced this pull request Nov 16, 2022
@rpavlik
OpenXR SDK 1.0.9 (2020-05-29)
97cfe49 
-   Registry
    -   Add an author ID, and reserve a vendor extension for Huawei.
        (OpenXR-Docs/KhronosGroup#46)
    -   Reserve vendor extensions for future LunarG overlay and input
        focus functionality. (internal MR 1720)
    -   Reserve vendor extensions for Microsoft. (internal MR 1723)
    -   Add XR_EXT_hand_tracking multi-vendor extension. (internal MR
        1554, internal issue 1266, internal issue 1267, internal issue
        1268, internal issue 1269)
    -   Add XR_HUAWEI_controller_interaction vendor extension.
        (OpenXR-Docs/KhronosGroup#47)
    -   Add XR_MNDX_egl_enable provisional vendor extension.
        (OpenXR-Docs/KhronosGroup#48)
    -   Add XR_MSFT_spatial_graph_bridge vendor extension. (internal MR
        1730)
    -   Add XR_MSFT_secondary_view_configuration and
        XR_MSFT_first_person_observer vendor extensions. (internal MR
        1731)
    -   Add XR_MSFT_hand_mesh_tracking vendor extension. (internal MR
        1736)
    -   Fix missing space in XML definition of
        XrSpatialAnchorCreateInfoMSFT. (internal MR 1742, internal issue
        1351, OpenXR-SDK-Source/KhronosGroup#187)
    -   Update a number of contacts for author/vendor tags. (internal MR
        1788, internal issue 1326)
-   SDK
    -   Replaced usage of the _DEBUG macro with NDEBUG. (internal MR
        1756)
    -   Allow disabling of std::filesystem usage via CMake, and detect
        if it’s available and what its requirements are.
        (OpenXR-SDK-Source/KhronosGroup#192, OpenXR-SDK-Source/KhronosGroup#188)
    -   CI: Modifications to Azure DevOps build pipeline. Now builds UWP
        loader DLLs in addition to Win32 loader DLLs. No longer builds
        static loader libraries due to linkability concerns. Re-arranged
        release artifact zip to distinguish architecture from 32-bit or
        64-bit.
    -   Loader: Replace global static initializers with functions that
        return static locals. With this change, code that includes
        OpenXR doesn’t have to page in this code and initialize these
        during startup. (OpenXR-SDK-Source/KhronosGroup#173)
    -   Loader: Unload runtime when xrCreateInstance fails. (internal MR
        1778)
    -   Loader: Add “info”-level debug messages listing all the places
        that we look for the OpenXR active runtime manifest.
        (OpenXR-SDK-Source/KhronosGroup#190)
    -   Validation Layer: Fix crash in dereferencing a nullptr optional
        array handle when the count > 0. (internal MR 1709,
        OpenXR-SDK-Source/KhronosGroup#161, internal issue 1322)
    -   Validation Layer: Fix static analysis error and possible loss of
        validation error. (internal MR 1715, OpenXR-SDK-Source/KhronosGroup#160,
        internal issue 1321)
    -   Validation Layer: Simplify some generated code, and minor
        performance improvements. (OpenXR-SDK-Source/KhronosGroup#176)
    -   API Dump Layer: Fix crash in dereferencing a nullptr while
        constructing a std::string. (internal MR 1712,
        OpenXR-SDK-Source/KhronosGroup#162, internal issue 1323)
    -   hello_xr: Fix releasing a swapchain image with the incorrect
        image layout. (internal MR 1755)
    -   hello_xr: Prefer VK_LAYER_KHRONOS_validation over
        VK_LAYER_LUNARG_standard_validation when available. (internal MR
        1755)
    -   hello_xr: Optimizations to D3D12 plugin to avoid GPU pipeline
        stall. (internal MR 1770) (OpenXR-SDK-Source/KhronosGroup#175)
    -   hello_xr: Fix build with Vulkan headers 1.2.136.
        (OpenXR-SDK-Source/KhronosGroup#181, OpenXR-SDK-Source/KhronosGroup#180, internal issue
        1347)
    -   hello_xr: Fix build with Visual Studio 16.6.
        (OpenXR-SDK-Source/KhronosGroup#186, OpenXR-SDK-Source/KhronosGroup#184)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpavlik rpavlik rpavlik requested changes

Assignees
No one assigned
Labels
waiting for reporter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philn @CLAassistant @rpavlik