MSVC support; [.github/workflows/cmake.yml] Add macOS and Windows building + testing #282

SamuelMarks · 2021-11-25T02:21:30Z

Added MSVC support, cross-platform testing in CI, and switched to more secure variants of C stdlib function (whence enabled or MSVC).

This closes #176, next is to close #251 (vcpkg is what I use for the rest of my deps).

…s; add vcpkg dependency; cache vcpkg; install pthreads with vcpkg; use vcpkg in cmake configuration

…every step; [libfswatch/CMakeLists.txt] Use `PThreads4W::PThreads4W` and `find_package` on MSVC variant

…c++/windows/cygwin_paths.cpp] Rename original for cygwin only; [src/libfswatch/c++/windows/win_paths.cpp] Use std functions to switch between std::wstring and std::string; [src/libfswatch/c++/windows/win_directory_change_event.hpp] Rename macro close comment

…++/windows/win_paths.cpp] Use different implementation

…cmake.yml] Invert slash ; [src/libfswatch/c++/windows/win_paths.cpp] Use different implementation; [src/libfswatch/c++/windows/win_paths.hpp] Use const ref; [src/libfswatch/c++/windows/cygwin_paths.cpp] Switch to const ref

…(MSVC)`

…ll}_monitor.cpp}] Use \ on Windows (MSVC) and / everywhere else

…eads.cmake] Init

…c\fswatch.cpp] Use windows optarg; use secure variants of `gmtime` & `localtime`; [fswatch\src\CMakeLists.txt] Add getopt dir/lib on MSVC; [fswatch\src\windows] getopt port for MSVC; [libfswatch] MSVC variants

…MSVC; [CMakeLists.txt] Enable getopt on msvc; [test/src/CMakeLists.txt] Add `source_group`s; [fswatch/CMakeLists.txt] Add `source_group`s; install headers to include; `add_subdirectory` for `PUBLIC` getopt; [fswatch/windows/CMakeLists.txt] Install properly

… MSVC; [fswatch\src\fswatch.cpp] Print error; [fswatch\src\libfswatch\c\cevent.h] Use `size_t` instead of `unsigned int` to match expected type; [fswatch\src\libfswatch\c++\windows\win_directory_change_event.cpp] Cast types; deduplicate mask; [fswatch\src\libfswatch\filter.cpp] Use `size_t` for index and length; [fswatch\src\libfswatch\monitor.cpp] print error; [fswatch\test\src\fswatch.c] Use `size_t` for `event_num`

…stall`; use `list(APPEND` rather than `set(<var> <var>`; [libfswatch/CMakeLists.txt] use explicit var over pattern matching for install

…present. These files can prevent the core C++ runtime and other packages from compiling correctly"; [fswatch/src/CMakeLists.txt] Only install headers to `include` and to `bin` when `BUILD_SHARED_LIBS` (as per vcpkg policy)

… to install headers to `include` and to `bin` (as per vcpkg policy)

…cies and include directories transitive, removing relative directories and non-install compatible paths in the process (via build expressions)

…res; [fswatch\src\windows] Use CMake approach to symbol exporting; [fswatch\CMakeLists.txt] `PTHREADS4W_FOUND` is the correct condition

…|.hpp file, turning them into libraries, and ensuring that all the libraries have the right dependencies added; use new namespace-free header `#include`s; [src\libfswatch\c\windows\realpath.c] Use CMake export macro; remove `__restrict__` (didn't seem to be supported in MSVC)

…KE_CURRENT_BINARY_DIR}; add ${CMAKE_CURRENT_BINARY_DIR} to `target_include_directories`; [libfswatch\src\libfswatch\c\windows\realpath.c] Use `_access` over `access` to quell POSIX compliancy warning from MSVC

…windows\win_directory_change_event.cpp] Do not include intrinsics (for x86 support)

…ly what is needed (gaining x86 Windows support in the process)

…VC` condition; [libfswatch/src/libfswatch/c++/*_monitor.cpp] Flatten `#include` namespace to match new one from multi-CMakeLists.txt refactor

…C_COMPILER_ARCHITECTURE_ID`; [src\libfswatch\c++\{path_utils,poll_monitor,windows/win_strings}.cpp] Guard intrinsics to only be included `#if defined(_X86_) || defined(_AMD64_)`

…TURE_ID` || `CMAKE_CXX_COMPILER_ARCHITECTURE_ID` || `CMAKE_SYSTEM_PROCESSOR`; [src\libfswatch\c++\windows\{win_directory_change_Event,win_error_message,win_handle}.cpp,win_handle.hpp,src\libfswatch\c++\windows_monitor.cpp] Guard intrinsics to only be included `#if defined(_X86_) || defined(_AMD64_)`

…ching what windows headers expect (tested on x86 and x64 so far)

…for; [*.cpp,win_handle.hpp] Use negative ARM check for including intrinsics

emcrisostomo

Thanks for your contribution! I think this is great work, and puts us very close to having both Autotools and CMake builds available.

But we're still not there: now, the Autotools build is broken (I've added some comments to the PR). Would you be willing to review this, so that we end up with a fully functioning and tested PR that still builds with the same behaviour in all the supported operating systems?

As is, I'm not able to pull it, and I'm not able to test your work on Windows.

Thanks!

emcrisostomo · 2022-06-14T13:41:24Z

fswatch/src/windows/CMakeLists.txt

@@ -0,0 +1,42 @@
+set(LIBRARY_NAME "getopt")


I understand why you're creating multiple libraries etc., but it's odd, if not wrong, that the GNU gettext files are inside a directory called windows.

emcrisostomo · 2022-06-14T13:42:45Z

fswatch/src/CMakeLists.txt

@@ -13,22 +13,41 @@
 # You should have received a copy of the GNU General Public License along with
 # this program.  If not, see <http://www.gnu.org/licenses/>.
 #
-set(FSWATCH_SRC_FILES
-        fswatch.cpp


This applies to many other places: why are we quoting strings when not required?

emcrisostomo · 2022-06-14T13:44:58Z

fswatch/src/fswatch.cpp

+#include "event.hpp"
+#include "monitor.hpp"
+#include "monitor_factory.hpp"
+#include "fsw_error.h"


If we rename files and change the include paths, we should keep the build system working, and autotools are now broken

emcrisostomo · 2022-06-14T13:46:43Z

libfswatch/src/libfswatch/c/windows/realpath.c

@@ -21,9 +21,9 @@
 #include <io.h>
 #include <stdlib.h>
 #include <errno.h>
+#include "cfswatch_export.h"


This file is not generated by the build system, so the build breaks when using the Autotools

emcrisostomo · 2022-06-14T13:48:41Z

fswatch/src/fswatch.cpp

-#include "libfswatch/c/libfswatch.h"
-#include "libfswatch/c/libfswatch_log.h"
-#include "libfswatch/c++/libfswatch_exception.hpp"
+#include "path_utils.hpp"


Using the library prefix was a conscious choice. Why would this refactor be required?

Can change to using hte full paths

emcrisostomo · 2022-06-14T13:50:53Z

libfswatch/src/libfswatch/c++/event.hpp

@@ -31,6 +31,7 @@
 #  include <vector>
 #  include <iostream>
 #  include "../c/cevent.h"
+#  include "cxxfswatch_export.h"


As in the C case, this file is not generated by the Autotools thus breaking the build

emcrisostomo · 2022-06-14T13:52:36Z

libfswatch/src/libfswatch/gettext/CMakeLists.txt

+# this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+set(LIBRARY_NAME "gettext")


All this does not belong to Gettext at all

emcrisostomo · 2022-06-14T13:53:11Z

CMakeLists.txt

@@ -14,13 +14,13 @@
 # this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 cmake_minimum_required(VERSION 3.8)
-project(fswatch VERSION 1.16.0 LANGUAGES C CXX)
+project(fs_watch VERSION 1.17.0 LANGUAGES C CXX)


emcrisostomo · 2022-06-14T13:53:58Z

CMakeLists.txt


 # Add option to choose between shared and static libraries
-option(BUILD_SHARED_LIBS "Build shared libraries" OFF)
+option(BUILD_SHARED_LIBS "Build shared libraries" ON)


Let's not change defaults people is relying on: the point of this setting is for anybody to override

emcrisostomo · 2022-06-14T13:54:31Z

CMakeLists.txt

@@ -44,7 +53,10 @@ include(CheckCXXSymbolExists)
 check_include_file_cxx(getopt.h HAVE_GETOPT_H)

 if (HAVE_GETOPT_H)
-    check_cxx_symbol_exists(getopt_long getopt.h HAVE_GETOPT_LONG)
+    check_cxx_symbol_exists(getopt_long "getopt.h" HAVE_GETOPT_LONG)


Could you explain this change with the else block?

SamuelMarks · 2022-06-14T15:15:48Z

@emcrisostomo I'll review your requested changes when I get the chance—my last commit was in February, first PR sent in November—and amend accordingly.

I can maintain compatibility with autotools (I suppose), for the symbol exporters I can create a pure C header file with the necessary ifdefs for each of gcc, clang, and msvc. If you want more than those compilers then I guess expand the list yourself. Alternatively; and preferably; Makefiles can be generated from CMake.

emmenlau · 2023-01-10T15:54:11Z

I'm actually very much interested in native MSVC support (without the need for Cygwin, dirent.h and other compatibility layers). Does this PR provide native MSVC support? @SamuelMarks , is it still actively pursued?

SamuelMarks · 2023-01-10T18:46:24Z

@emmenlau There were a lot of changes requested and compatibility with existing autotools toolchain required.

If I get the chance will make the edits and update to latest master in the process. But really would like a more complete test suite to confirm this all works (not to mention the delayed responses in feedback on the PR)

SamuelMarks added 7 commits November 24, 2021 21:16

[.github/workflows/cmake.yml] Add macOS and Windows building + testing

e655032

[.github/workflows/cmake.yml] Bifurcate into macOS & Linux and Window…

cf1f760

…s; add vcpkg dependency; cache vcpkg; install pthreads with vcpkg; use vcpkg in cmake configuration

[.github/workflows/cmake.yml] Install gettext with vcpkg; use cmd on …

a7df780

…every step; [libfswatch/CMakeLists.txt] Use `PThreads4W::PThreads4W` and `find_package` on MSVC variant

[.github/workflows/cmake.yml] Rework caching name ; [src/libfswatch/c…

b0ce833

…++/windows/win_paths.cpp] Use different implementation

[libfswatch/CMakeLists.txt] Move cygwin_paths to else block of `if …

e90581c

…(MSVC)`

SamuelMarks changed the title ~~[.github/workflows/cmake.yml] Add macOS and Windows building + testing~~ MSVC support; [.github/workflows/cmake.yml] Add macOS and Windows building + testing Nov 25, 2021

SamuelMarks added 9 commits November 25, 2021 11:25

[libfswatch/src/libfswatch/c++/{path_utils.hpp,{fen,inotify,kqueue,po…

facc417

…ll}_monitor.cpp}] Use \ on Windows (MSVC) and / everywhere else

[libfswatch\src\libfswatch\c++\windows] Port to MSVC; [cmake\FindPthr…

2cb8e87

…eads.cmake] Init

[.github\workflows\cmake.yml] Set VCPKG_TARGET_TRIPLET; [fswatch\sr…

719d31a

…c\fswatch.cpp] Use windows optarg; use secure variants of `gmtime` & `localtime`; [fswatch\src\CMakeLists.txt] Add getopt dir/lib on MSVC; [fswatch\src\windows] getopt port for MSVC; [libfswatch] MSVC variants

[fswatch/src/fswatch.cpp] Make signal calls equivalent to sigaction

e2d0825

[fswatch/src/fswatch.cpp] Add missing type to tm_time

146d7d2

[README.windows] Add MSVC instructions

5910ad4

Merge remote-tracking branch 'origin/multi-os-ci' into multi-os-ci

6b273e1

SamuelMarks mentioned this pull request Dec 23, 2021

cmake build issue #176

Open

SamuelMarks added 12 commits January 14, 2022 14:37

[**/CMakeLists.txt] Quote all vars

f5b9044

[m4/{lib}fswatch_version.m4,po/fswatch.pot,CMakeLists.txt] Bump version

9576be7

[{fswatch/src,libfswatch}/CMakeLists.txt] Fix iconv support within vcpkg

ee32afd

[{fswatch/src,fswatch/src/windows,libfswatch}/CMakeLists.txt] Fix `in…

9e0e2c4

…stall`; use `list(APPEND` rather than `set(<var> <var>`; [libfswatch/CMakeLists.txt] use explicit var over pattern matching for install

[fswatch/src/CMakeLists.txt] Explicitly check VCPKG_LIBRARY_LINKAGE…

025ec52

… to install headers to `include` and to `bin` (as per vcpkg policy)

[**/CMakeLists.txt] Major fixup and refactor to make targets dependen…

f788575

…cies and include directories transitive, removing relative directories and non-install compatible paths in the process (via build expressions)

[libfswatch\src\libfswatch\c\*.h] Export symbols; comment macro closu…

271eb5a

…res; [fswatch\src\windows] Use CMake approach to symbol exporting; [fswatch\CMakeLists.txt] `PTHREADS4W_FOUND` is the correct condition

[CMakeLists.txt] generate_export_header to the default, i.e., ${CMA…

f2c98cc

…KE_CURRENT_BINARY_DIR}; add ${CMAKE_CURRENT_BINARY_DIR} to `target_include_directories`; [libfswatch\src\libfswatch\c\windows\realpath.c] Use `_access` over `access` to quell POSIX compliancy warning from MSVC

[CMakeLists.txt] Set /EHsc on MSVC; [libfswatch\src\libfswatch\c++\…

c24f9de

…windows\win_directory_change_event.cpp] Do not include intrinsics (for x86 support)

[*.cpp,*.hpp] Remove #include <windows.h> in favour of including on…

ad8228b

…ly what is needed (gaining x86 Windows support in the process)

SamuelMarks added 6 commits January 17, 2022 20:46

[libfswatch/src/libfswatch/c/CMakeLists.txt] Guard "/EHsc" with `MS…

045f44c

…VC` condition; [libfswatch/src/libfswatch/c++/*_monitor.cpp] Flatten `#include` namespace to match new one from multi-CMakeLists.txt refactor

[CMakeLists.txt] Switch from CMAKE_HOST_SYSTEM_PROCESSOR to `CMAKE_…

4de20b8

…C_COMPILER_ARCHITECTURE_ID`; [src\libfswatch\c++\{path_utils,poll_monitor,windows/win_strings}.cpp] Guard intrinsics to only be included `#if defined(_X86_) || defined(_AMD64_)`

[CMakeLists.txt] Carefully parse TARGET_ARCH so that its choice mat…

d30c92f

…ching what windows headers expect (tested on x86 and x64 so far)

[CMakeLists.txt] TARGET_ARCH now matches what Windows header looks …

ca7c03d

…for; [*.cpp,win_handle.hpp] Use negative ARM check for including intrinsics

[**CMakeLists.txt] Export targets for consumption by find_package

99d52cb

emcrisostomo requested changes Jun 14, 2022

View reviewed changes

emcrisostomo mentioned this pull request Jun 14, 2022

Vcpkg support (Windows package manager) #251

Open

emcrisostomo force-pushed the master branch 2 times, most recently from a994138 to 70fe7a5 Compare June 17, 2022 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSVC support; [.github/workflows/cmake.yml] Add macOS and Windows building + testing #282

MSVC support; [.github/workflows/cmake.yml] Add macOS and Windows building + testing #282

SamuelMarks commented Nov 25, 2021 •

edited

Loading

emcrisostomo left a comment

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

SamuelMarks Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

emcrisostomo Jun 14, 2022

SamuelMarks commented Jun 14, 2022

emmenlau commented Jan 10, 2023

SamuelMarks commented Jan 10, 2023

MSVC support; [.github/workflows/cmake.yml] Add macOS and Windows building + testing #282

Are you sure you want to change the base?

MSVC support; [.github/workflows/cmake.yml] Add macOS and Windows building + testing #282

Conversation

SamuelMarks commented Nov 25, 2021 • edited Loading

emcrisostomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamuelMarks commented Jun 14, 2022

emmenlau commented Jan 10, 2023

SamuelMarks commented Jan 10, 2023

SamuelMarks commented Nov 25, 2021 •

edited

Loading