Portability and Build System Cleanup #192

rpavlik · 2020-05-13T21:01:24Z

I've extracted some changes from another PR where I'm hitting issues, in hopes of merging all but the really tricky stuff.

Highlights:

Nearly-usable port to MinGW (works if you turn off building hello_xr) as well as cross-compiling with Clang and mingw on Linux.
Fixes enabling a bunch of warnings in the build process - there was a small typo preventing the code that should have enabled warnings in CMake from actually doing it. (This means that Linux CI should now be about as strict as AppVeyor, hooray!)
Better support of std::filesystem - Think this should be able to replace cmake: Check for C++-17 and conditionally enable it #188 (thank you for submitting that!) in a generic way

philn · 2020-05-14T09:41:14Z

@rpavlik I've tried your buildsys branch here, without success though:

    cmake -B_builddir -H. -G"Ninja" -DCMAKE_INSTALL_PREFIX:PATH="/usr" \
    -DCMAKE_INSTALL_LIBDIR:PATH="lib/x86_64-linux-gnu"  -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_TESTS=OFF
-- The C compiler identification is GNU 9.3.0
-- The CXX compiler identification is GNU 9.3.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.7.4", minimum required is "3") 
-- Performing Test HAVE_FILESYSTEM_IN_STD
-- Performing Test HAVE_FILESYSTEM_IN_STD - Failed
-- Performing Test HAVE_FILESYSTEM_IN_STDEXPERIMENTAL
-- Performing Test HAVE_FILESYSTEM_IN_STDEXPERIMENTAL - Failed
-- Performing Test HAVE_FILESYSTEM_IN_STD_17
-- Performing Test HAVE_FILESYSTEM_IN_STD_17 - Failed
-- Performing Test HAVE_FILESYSTEM_WITHOUT_LIB
-- Performing Test HAVE_FILESYSTEM_WITHOUT_LIB - Failed
-- Performing Test HAVE_FILESYSTEM_NEEDING_LIB
-- Performing Test HAVE_FILESYSTEM_NEEDING_LIB - Failed
-- Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY OPENGL_glx_LIBRARY OPENGL_INCLUDE_DIR) 
-- Could NOT find VulkanHeaders (missing: VulkanHeaders_INCLUDE_DIR) 
-- Could NOT find VulkanRegistry (missing: VulkanRegistry_DIR) 
-- Could NOT find Vulkan (missing: Vulkan_LIBRARY Vulkan_INCLUDE_DIR) 
-- Looking for pthread.h
-- Looking for pthread.h - not found
CMake Error at /usr/share/cmake-3.15/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find Threads (missing: Threads_FOUND)
Call Stack (most recent call first):
  /usr/share/cmake-3.15/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /usr/share/cmake-3.15/Modules/FindThreads.cmake:220 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  src/CMakeLists.txt:58 (find_package)


-- Configuring incomplete, errors occurred!

rpavlik · 2020-05-14T17:18:52Z

And it built before with your branch? I'm very surprised you got it to build if CMake can't find threads.

It is weird that all 4 filesystem checks failed. Can you attach CMakeFiles/CMakeError.log from your build directory?

I'm installing arch linux in a VM here so I can repro this locally, in theory, but I'm concerned that there are now 2 build breaks on your system (though, the lack of filesystem just falls back to raw file apis).

rpavlik · 2020-05-14T19:28:46Z

Hmm, so I couldn't reproduce in Arch: had a very minimum install, but it found threads as well as std::filesystem (but only in experimental mode)

$ cmake -S . -B build -G Ninja
-- The C compiler identification is GNU 10.1.0
-- The CXX compiler identification is GNU 10.1.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc - works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ - works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.8.2", minimum required is "3") 
-- Performing Test HAVE_FILESYSTEM_IN_STD
-- Performing Test HAVE_FILESYSTEM_IN_STD - Failed
-- Performing Test HAVE_FILESYSTEM_IN_STDEXPERIMENTAL
-- Performing Test HAVE_FILESYSTEM_IN_STDEXPERIMENTAL - Success
-- Performing Test HAVE_FILESYSTEM_IN_STD_17
-- Performing Test HAVE_FILESYSTEM_IN_STD_17 - Failed
-- Performing Test HAVE_FILESYSTEM_WITHOUT_LIB
-- Performing Test HAVE_FILESYSTEM_WITHOUT_LIB - Failed
-- Performing Test HAVE_FILESYSTEM_NEEDING_LIB
-- Performing Test HAVE_FILESYSTEM_NEEDING_LIB - Success
-- Found OpenGL: /usr/lib/libOpenGL.so   
-- Enabling OpenGL support
-- Could NOT find VulkanHeaders (missing: VulkanHeaders_INCLUDE_DIR) 
CMake Warning (dev) at /usr/share/cmake-3.17/Modules/FindPackageHandleStandardArgs.cmake:272 (message):
  The package name passed to `find_package_handle_standard_args`
  (VulkanRegistry) does not match the name of the calling package
  (VulkanHeaders).  This can lead to problems in calling code that expects
  `find_package` result variables (e.g., `_FOUND`) to follow a certain
  pattern.
Call Stack (most recent call first):
  src/cmake/FindVulkanHeaders.cmake:84 (find_package_handle_standard_args)
  src/CMakeLists.txt:48 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Could NOT find VulkanRegistry (missing: VulkanRegistry_DIR) 
-- Could NOT find Vulkan (missing: Vulkan_INCLUDE_DIR) 
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE  
-- Found JsonCpp: /usr/include  
-- Enabling OpenGL support in hello_xr, loader_test, and conformance, if configured
-- Presentation backend selected for hello_xr, loader_test, conformance: xlib
-- Found X11: /usr/include   
-- Looking for XOpenDisplay in /usr/lib/libX11.so;/usr/lib/libXext.so
-- Looking for XOpenDisplay in /usr/lib/libX11.so;/usr/lib/libXext.so - found
-- Looking for gethostbyname
-- Looking for gethostbyname - found
-- Looking for connect
-- Looking for connect - found
-- Looking for remove
-- Looking for remove - found
-- Looking for shmat
-- Looking for shmat - found
-- Looking for IceConnectionNumber in ICE
-- Looking for IceConnectionNumber in ICE - found
-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
-- BUILD_WITH_XLIB_HEADERS: ON
-- BUILD_WITH_XCB_HEADERS: OFF
-- BUILD_WITH_WAYLAND_HEADERS: OFF
-- Found glslc: /usr/bin/glslc
-- Looking for secure_getenv
-- Looking for secure_getenv - found
-- Looking for __secure_getenv
-- Looking for __secure_getenv - not found
-- Performing Test SUPPORTS_Wall
-- Performing Test SUPPORTS_Wall - Success
-- Performing Test SUPPORTS_C_Wall
-- Performing Test SUPPORTS_C_Wall - Success
-- Performing Test SUPPORTS_Werrorunusedparameter
-- Performing Test SUPPORTS_Werrorunusedparameter - Success
-- Performing Test SUPPORTS_C_Werrorunusedparameter
-- Performing Test SUPPORTS_C_Werrorunusedparameter - Success
-- Performing Test SUPPORTS_Werrorunusedargument
-- Performing Test SUPPORTS_Werrorunusedargument - Failed
-- Performing Test SUPPORTS_C_Werrorunusedargument
-- Performing Test SUPPORTS_C_Werrorunusedargument - Failed
-- Performing Test SUPPORTS_Wpointerarith
-- Performing Test SUPPORTS_Wpointerarith - Success
-- Performing Test SUPPORTS_C_Wpointerarith
-- Performing Test SUPPORTS_C_Wpointerarith - Success
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ryan/src/OpenXR-SDK-Source/build

so more info on your environment, etc. would be very helpful.

philn · 2020-05-15T08:47:20Z

And it built before with your branch?

Of course it did :)

I'm very surprised you got it to build if CMake can't find threads.

It is weird that all 4 filesystem checks failed. Can you attach CMakeFiles/CMakeError.log from your build directory?

CMakeError.log

I'm installing arch linux in a VM here so I can repro this locally, in theory, but I'm concerned that there are now 2 build breaks on your system (though, the lack of filesystem just falls back to raw file apis).

I see you use GCC 10 though. Here I use 9.3.0.

philn

The checks fail here because the -std=c++17 option isn't passed to the compiler.

philn · 2020-05-15T08:53:24Z

src/cmake/StdFilesystemFlags.cmake

+set(_stdfs_test_source
+    "bool is_reg_file(const std::string &path) { return fs::is_regular_file(path); }
+int main() {
+    (void)is_reg_file(\"/etc/os-release\");


I'm not sure to understand this. Here CMake checks a file exists? I surely doesn't in my build chroot. Assuming /etc/os-release exists everywhere doesn't seem correct.

No, this is never actually run. This is just code I had previously used to reproduce an issue with the stdc++fs library not working, so I knew it doesn't link without the same things in STD:: filesystem that the loader uses. I just had to make sure it looked real enough so it wasn't getting optimized out.

philn · 2020-05-16T11:35:04Z

Ok with this additional patch it builds here now with GCC 9.3.0 :)

diff --git a/src/cmake/StdFilesystemFlags.cmake b/src/cmake/StdFilesystemFlags.cmake
index ded2da1..ec3001f 100644
--- a/src/cmake/StdFilesystemFlags.cmake
+++ b/src/cmake/StdFilesystemFlags.cmake
@@ -72,6 +72,7 @@ set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)
 check_cxx_source_compiles("${_stdfs_needlib_source}" HAVE_FILESYSTEM_WITHOUT_LIB)
 set(CMAKE_REQUIRED_LIBRARIES stdc++fs)
 check_cxx_source_compiles("${_stdfs_needlib_source}" HAVE_FILESYSTEM_NEEDING_LIB)
+unset(CMAKE_REQUIRED_LIBRARIES)
 
 function(openxr_add_filesystem_utils TARGET_NAME)
     target_sources(${TARGET_NAME} PRIVATE ${PROJECT_SOURCE_DIR}/src/common/filesystem_utils.cpp)

rpavlik · 2020-05-18T17:25:22Z

OK, I updated it with more unsets :)

philn · 2020-05-19T14:18:14Z

Thanks @rpavlik ! I confirm this PR builds fine here with GCC 9.3.0 :)

rpavlik · 2020-05-19T19:32:59Z

Great. I tried it on Visual Studio 2019, which was only partially succesful - it's not detecting the presence of filesystem. Turns out the ifdefs are more complex than I remembered. Think I will probably either pull all other commits from here into a separate PR, or put in a small bodge to avoid skipping std::fs usage in msvc until I have a chance to do it better.

This should transparently identify when it's available in standard or experimental forms, as well as when it requires stdc++fs to be linked. MSVC was a bit weird to get working here, but eventually got it.

hello_xr D3D plugin requires DirectXColors which isn't in MinGW, so we must check for that too. Fixes build of hello_xr on MinGW

…vered.

rpavlik · 2020-05-28T22:20:15Z

OK, got msvc working right now. Will merge once the CI passes.

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

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

rpavlik added 10 commits May 13, 2020 11:52

build: Clean up recent expansion of Windows-related platforms

a973163

loader, hello_xr: case fixes in includes

382fe86

loader_platform: GCC wants a cast

47cd184

build: Windows does not imply MSVC

c412259

gl_format: Fix duplicate definition

1d597d8

build: Choose path separator based on host system, not target.

2c625b5

loader.rc: Enable build on MinGW

4a0b723

threading: Fix build on MinGW

8a74948

list test: Fix build on MinGW

eab3078

build: Be sure to enable threads for layers

fb885a3

rpavlik requested review from brycehutchings and yl-msft May 13, 2020 21:01

rpavlik force-pushed the buildsys branch from 61d8736 to 52e54b9 Compare May 13, 2020 21:02

rpavlik mentioned this pull request May 13, 2020

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

Closed

rpavlik force-pushed the buildsys branch from 52e54b9 to 6482076 Compare May 13, 2020 21:32

rpavlik mentioned this pull request May 13, 2020

Mingw #153

Open

philn reviewed May 15, 2020

View reviewed changes

rpavlik force-pushed the buildsys branch from 6482076 to 02202c6 Compare May 15, 2020 16:31

rpavlik force-pushed the buildsys branch from 02202c6 to e8b1f32 Compare May 18, 2020 16:55

rpavlik force-pushed the buildsys branch from 5354781 to 1e297cd Compare May 28, 2020 21:58

rpavlik added 2 commits May 28, 2020 16:59

build: Better detect (and permit disabling) std::filesystem in CMake.

6d69b00

This should transparently identify when it's available in standard or experimental forms, as well as when it requires stdc++fs to be linked. MSVC was a bit weird to get working here, but eventually got it.

build: Be more precise in actually searching for D3D11/12

e69ea84

hello_xr D3D plugin requires DirectXColors which isn't in MinGW, so we must check for that too. Fixes build of hello_xr on MinGW

rpavlik added 5 commits May 28, 2020 16:59

build: Remove redundant linking

d23a448

loader: Remove unused parameters

2682316

build: Fix cross-compile of loader embedded jsoncpp

0fde7a9

build: Fix enabling of build warnings, and handle most warnings disco…

ca37dc8

…vered.

Update gitignore

9df5811

rpavlik force-pushed the buildsys branch from 1e297cd to 9df5811 Compare May 28, 2020 22:00

filesystem_utils: Add missing include

f438ea2

rpavlik merged commit b38a237 into KhronosGroup:master May 28, 2020

rpavlik deleted the buildsys branch May 28, 2020 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portability and Build System Cleanup #192

Portability and Build System Cleanup #192

rpavlik commented May 13, 2020 •

edited

Loading

philn commented May 14, 2020

rpavlik commented May 14, 2020 •

edited

Loading

rpavlik commented May 14, 2020

philn commented May 15, 2020 •

edited

Loading

philn left a comment

philn May 15, 2020

rpavlik May 15, 2020

philn commented May 16, 2020

rpavlik commented May 18, 2020

philn commented May 19, 2020

rpavlik commented May 19, 2020

rpavlik commented May 28, 2020

Portability and Build System Cleanup #192

Portability and Build System Cleanup #192

Conversation

rpavlik commented May 13, 2020 • edited Loading

philn commented May 14, 2020

rpavlik commented May 14, 2020 • edited Loading

rpavlik commented May 14, 2020

philn commented May 15, 2020 • edited Loading

philn left a comment

Choose a reason for hiding this comment

philn May 15, 2020

Choose a reason for hiding this comment

rpavlik May 15, 2020

Choose a reason for hiding this comment

philn commented May 16, 2020

rpavlik commented May 18, 2020

philn commented May 19, 2020

rpavlik commented May 19, 2020

rpavlik commented May 28, 2020

rpavlik commented May 13, 2020 •

edited

Loading

rpavlik commented May 14, 2020 •

edited

Loading

philn commented May 15, 2020 •

edited

Loading