Windows build & test CI and fixes

.github/workflows/build_test.yml

@@ -28,23 +28,48 @@ jobs:
       - name: Checking code style
         run: ./scripts/run-clang-tidy.sh
 
-  build_and_test:
+  ubuntu-build-and-test:
     runs-on: ubuntu-latest
 
     steps:
       - uses: actions/checkout@v3
         with:
           submodules: recursive
+
       - name: Setup Ubuntu
+        run: ./scripts/setup-ubuntu.sh
+
+      - name: Build
         run: |
-          ./scripts/setup-ubuntu.sh
           mkdir build
-      - name: Run cmake
-        run: |
           cmake --version
           cmake -Bbuild -GNinja -DCMAKE_BUILD_TYPE=Debug -DBUILD_TZ_LIB=ON
-      - name: Build
-        run: ninja -C build
+          ninja -C build
+
       - name: Test
         run: ctest --test-dir build --output-on-failure --timeout 30
 
+  windows-build-and-test:
+    runs-on: windows-latest
+
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          submodules: recursive
+
+      - name: Set up JDK 11
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'
+          java-version: '11'
+
+      - name: Build
+        run: |
+          ./scripts/find_vs.ps1
+          mkdir build
+          cmake --version
+          cmake -Bbuild -GNinja -DCMAKE_BUILD_TYPE=Debug -DBUILD_TZ_LIB=ON
+          ninja -C build
+
+      - name: Test
+        run: ctest --test-dir build --output-on-failure --timeout 30

CMakeLists.txt

@@ -41,9 +41,10 @@ set(CMAKE_CXX_STANDARD 17)
 add_definitions(-DANTLR4CPP_STATIC)
 # using /MD flag for antlr4_runtime (for Visual C++ compilers only)
 set(ANTLR4_WITH_STATIC_CRT OFF)
-set(ANTLR4_TAG 4.13.2)
-set(ANTLR4_ZIP_REPOSITORY
-    https://github.com/antlr/antlr4/archive/refs/tags/${ANTLR4_TAG}.zip)
+set(ANTLR4_BUILD_CPP_TESTS OFF)
+# Note: df4d68c adds a fix for MSVC compilers. No release has been made since;
+# latest release was 4.13.2. Revert back to a tag once 4.13.3 is released.
+set(ANTLR4_TAG df4d68c09cdef73e023b8838a8bc7ca4dff1d1de)
 include(ExternalAntlr4Cpp)
 include_directories(${ANTLR4_INCLUDE_DIRS})
 set(ANTLR_EXECUTABLE_DIR ${CMAKE_CURRENT_BINARY_DIR})

include/substrait/common/Io.h

@@ -30,12 +30,15 @@ enum class PlanFileFormat {
  * amount of memory that it consumed on disk.
  *
  * \param input_filename The filename containing the plan to convert.
+ * \param force_binary If true, the plan will be opened as a binary file.
+ *  Required on Windows to avoid text mode line-ending translation.
  * \return If loading was successful, returns a plan.  If loading was not
  * successful this is a status containing a list of parse errors in the status's
  * message.
  */
 absl::StatusOr<::substrait::proto::Plan> loadPlan(
-    std::string_view input_filename);
+    std::string_view input_filename,
+    bool force_binary = false);
 
 /*
  * \brief Writes the provided plan to disk.

scripts/find_vs.ps1

@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: Apache-2.0
+# Find and enter a Visual Studio development environment.
+# Required to use Ninja instead of msbuild on our build agents.
+function Enter-VsDevEnv {
+    [CmdletBinding()]
+    param(
+        [Parameter()]
+        [switch]$Prerelease,
+        [Parameter()]
+        [string]$architecture = "x64"
+    )
+
+    $ErrorActionPreference = 'Stop'
+
+    if ($null -eq (Get-InstalledModule -name 'VSSetup' -ErrorAction SilentlyContinue)) {
+        Install-Module -Name 'VSSetup' -Scope CurrentUser -SkipPublisherCheck -Force
+    }
+    Import-Module -Name 'VSSetup'
+
+    Write-Verbose 'Searching for VC++ instances'
+    $vsinfo = `
+        Get-VSSetupInstance  -All -Prerelease:$Prerelease `
+    | Select-VSSetupInstance `
+        -Latest -Product * `
+        -Require 'Microsoft.VisualStudio.Component.VC.Tools.x86.x64'
+
+    $vspath = $vsinfo.InstallationPath
+
+    switch ($env:PROCESSOR_ARCHITECTURE) {
+        "amd64" { $hostarch = "x64" }
+        "x86" { $hostarch = "x86" }
+        "arm64" { $hostarch = "arm64" }
+        default { throw "Unknown architecture: $switch" }
+    }
+
+    $devShellModule = "$vspath\Common7\Tools\Microsoft.VisualStudio.DevShell.dll"
+
+    Import-Module -Global -Name $devShellModule
+
+    Write-Verbose 'Setting up environment variables'
+    Enter-VsDevShell -VsInstanceId $vsinfo.InstanceId  -SkipAutomaticLocation `
+        -devCmdArguments "-arch=$architecture -host_arch=$hostarch"
+
+    Set-Item -Force -path "Env:\Platform" -Value $architecture
+
+    remove-Module Microsoft.VisualStudio.DevShell, VSSetup
+}
+
+Enter-VsDevEnv

src/substrait/common/Io.cpp

@@ -37,8 +37,10 @@ PlanFileFormat detectFormat(std::string_view content) {
 } // namespace
 
 absl::StatusOr<::substrait::proto::Plan> loadPlan(
-    std::string_view input_filename) {
-  auto contentOrError = textplan::readFromFile(input_filename.data());
+    std::string_view input_filename,
+    bool forceBinary) {
+  auto contentOrError =
+      textplan::readFromFile(input_filename.data(), forceBinary);
   if (!contentOrError.ok()) {
     return contentOrError.status();
   }

src/substrait/common/tests/IoTest.cpp

@@ -52,7 +52,8 @@ class SaveAndLoadTestFixture : public ::testing::TestWithParam<PlanFileFormat> {
                           std::filesystem::path("my_temp_dir"))
                              .string();
 
-    if (!std::filesystem::create_directory(testFileDirectory_)) {
+    std::filesystem::create_directory(testFileDirectory_);
+    if (!std::filesystem::exists(testFileDirectory_)) {
       ASSERT_TRUE(false) << "Failed to create temporary directory.";
       testFileDirectory_.clear();
     }
@@ -87,7 +88,15 @@ TEST_P(SaveAndLoadTestFixture, SaveAndLoad) {
   auto status = ::io::substrait::savePlan(plan, tempFilename, encoding);
   ASSERT_TRUE(status.ok()) << "Save failed.\n" << status;
 
+#ifdef _WIN32
+  // Windows cannot rely on io::substrait::loadPlan to detect the file format,
+  // since it needs to a-priori specify how the file should be loaded.
+  bool forceBinary = encoding == PlanFileFormat::kBinary;
+  auto result = ::io::substrait::loadPlan(tempFilename, forceBinary);
+#else
   auto result = ::io::substrait::loadPlan(tempFilename);
+#endif
+
   ASSERT_TRUE(result.ok()) << "Load failed.\n" << result.status();
   ASSERT_THAT(
       *result,

src/substrait/textplan/converter/LoadBinary.cpp

@@ -42,17 +42,24 @@ class StringErrorCollector : public google::protobuf::io::ErrorCollector {
 
 } // namespace
 
-absl::StatusOr<std::string> readFromFile(std::string_view msgPath) {
-  std::ifstream textFile(std::string{msgPath});
-  if (textFile.fail()) {
+absl::StatusOr<std::string> readFromFile(
+    std::string_view msgPath,
+    bool forceBinary) {
+  std::ifstream file;
+  if (forceBinary)
+    file.open(std::string{msgPath}, std::ios::binary);
+  else
+    file.open(std::string{msgPath}, std::ios::in);
+
+  if (file.fail()) {
     auto currDir = std::filesystem::current_path().string();
     return absl::ErrnoToStatus(
         errno,
         fmt::format(
             "Failed to open file {} when running in {}", msgPath, currDir));
   }
   std::stringstream buffer;
-  buffer << textFile.rdbuf();
+  buffer << file.rdbuf();
   return buffer.str();
 }
 

src/substrait/textplan/converter/LoadBinary.h

@@ -12,8 +12,11 @@ class Plan;
 
 namespace io::substrait::textplan {
 
-// Read the contents of a file from disk.
-absl::StatusOr<std::string> readFromFile(std::string_view msgPath);
+// Read the contents of a file from disk. 'forceBinary' enables file reading in
+// binary mode.
+absl::StatusOr<std::string> readFromFile(
+    std::string_view msgPath,
+    bool forceBinary = false);
 
 // Reads a plan from a json-encoded text proto.
 // Returns a list of errors if the file cannot be parsed.

src/substrait/textplan/converter/SaveBinary.cpp

@@ -27,24 +27,18 @@ namespace io::substrait::textplan {
 absl::Status savePlanToBinary(
     const ::substrait::proto::Plan& plan,
     std::string_view output_filename) {
-  int outputFileDescriptor =
-      creat(std::string{output_filename}.c_str(), S_IREAD | S_IWRITE);
-  if (outputFileDescriptor == -1) {
-    return absl::ErrnoToStatus(
-        errno,
+  // Open file in binary mode and get its file descriptor
+  std::ofstream of(std::string{output_filename}, std::ios::binary);
+  if (!of) {
+    return absl::InternalError(
         fmt::format("Failed to open file {} for writing", output_filename));
   }
-  auto stream =
-      new google::protobuf::io::FileOutputStream(outputFileDescriptor);
 
-  if (!plan.SerializeToZeroCopyStream(stream)) {
+  if (!plan.SerializeToOstream(&of)) {
     return ::absl::UnknownError("Failed to write plan to stream.");
   }
 
-  if (!stream->Close()) {
-    return absl::AbortedError("Failed to close file descriptor.");
-  }
-  delete stream;
+  of.close();
   return absl::OkStatus();
 }
 

src/substrait/textplan/tests/ParseResultMatchers.cpp

@@ -149,31 +149,34 @@ class HasSymbolsMatcher {
 
   bool MatchAndExplain(const ParseResult& result, std::ostream* listener)
       const {
-    auto actualSymbols = symbolNames(result.getSymbolTable().getSymbols());
+    // Note: Need set or sorted vector for set_difference.
+    auto actualSymbolsSorted =
+        symbolNames(result.getSymbolTable().getSymbols());
+    std::sort(actualSymbolsSorted.begin(), actualSymbolsSorted.end());
+    std::vector<std::string> extraSymbols;
+    auto expectedSymbolsSorted = expectedSymbols_;
+    std::sort(expectedSymbolsSorted.begin(), expectedSymbolsSorted.end());
     if (listener != nullptr) {
-      std::vector<std::string> extraSymbols(actualSymbols.size());
       auto end = std::set_difference(
-          actualSymbols.begin(),
-          actualSymbols.end(),
-          expectedSymbols_.begin(),
-          expectedSymbols_.end(),
-          extraSymbols.begin());
-      extraSymbols.resize(end - extraSymbols.begin());
+          actualSymbolsSorted.begin(),
+          actualSymbolsSorted.end(),
+          expectedSymbolsSorted.begin(),
+          expectedSymbolsSorted.end(),
+          std::back_inserter(extraSymbols));
       if (!extraSymbols.empty()) {
         *listener << std::endl << "          with extra symbols: ";
         for (const auto& symbol : extraSymbols) {
           *listener << " \"" << symbol << "\"";
         }
       }
 
-      std::vector<std::string> missingSymbols(expectedSymbols_.size());
+      std::vector<std::string> missingSymbols;
       end = std::set_difference(
-          expectedSymbols_.begin(),
-          expectedSymbols_.end(),
-          actualSymbols.begin(),
-          actualSymbols.end(),
-          missingSymbols.begin());
-      missingSymbols.resize(end - missingSymbols.begin());
+          expectedSymbolsSorted.begin(),
+          expectedSymbolsSorted.end(),
+          actualSymbolsSorted.begin(),
+          actualSymbolsSorted.end(),
+          std::back_inserter(missingSymbols));
       if (!missingSymbols.empty()) {
         if (!extraSymbols.empty()) {
           *listener << ", and missing symbols: ";
@@ -185,7 +188,7 @@ class HasSymbolsMatcher {
         }
       }
     }
-    return actualSymbols == expectedSymbols_;
+    return actualSymbolsSorted == expectedSymbolsSorted;
   }
 
   void DescribeTo(std::ostream* os) const {

third_party/CMakeLists.txt

@@ -31,6 +31,28 @@ if(NOT ${GTEST_FOUND})
     OVERRIDE_FIND_PACKAGE)
   fetchcontent_makeavailable(GTest)
 endif()
+if(MSVC)
+  # ------------------------------------------------------------------------------
+  # gtest MSVC fix
+  # ------------------------------------------------------------------------------
+  # For some reason, googletest has include path issues when built with MSVC.
+  # Specifically, this seems like some incorrect assumptions about include paths
+  # inside the gmock project.
+  # We can fix this by injecting the include paths here.
+  function(fix_gtest_include TARGET)
+    target_include_directories(
+      ${TARGET}
+      PUBLIC $<BUILD_INTERFACE:${gtest_SOURCE_DIR}>
+             $<BUILD_INTERFACE:${gtest_SOURCE_DIR}/include>
+             $<BUILD_INTERFACE:${gtest_SOURCE_DIR}/googletest>
+             $<BUILD_INTERFACE:${gtest_SOURCE_DIR}/googletest/include>
+             $<INSTALL_INTERFACE:include/${TARGET}>)
+  endfunction()
+  set(gtest_erroneous_targets gmock gmock_main)
+  foreach(target ${gtest_erroneous_targets})
+    fix_gtest_include(${target})
+  endforeach()
+endif()
 
 set(PREVIOUS_BUILD_TESTING ${BUILD_TESTING})
 set(BUILD_TESTING OFF)

third_party/antlr4/cmake/ExternalAntlr4Cpp.cmake

@@ -100,8 +100,7 @@ if(ANTLR4_ZIP_REPOSITORY)
           -DDISABLE_WARNINGS:BOOL=ON
           -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=ON
           -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
-          # -DCMAKE_CXX_STANDARD:STRING=17 # if desired, compile the runtime with a different C++ standard
-          # -DCMAKE_CXX_STANDARD:STRING=${CMAKE_CXX_STANDARD} # alternatively, compile the runtime with the same C++ standard as the outer project
+          -DCMAKE_CXX_STANDARD:STRING=${CMAKE_CXX_STANDARD}
       INSTALL_COMMAND ""
       EXCLUDE_FROM_ALL 1)
 else()
@@ -122,8 +121,7 @@ else()
           -DDISABLE_WARNINGS:BOOL=ON
           -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=ON
           -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
-          # -DCMAKE_CXX_STANDARD:STRING=17 # if desired, compile the runtime with a different C++ standard
-          # -DCMAKE_CXX_STANDARD:STRING=${CMAKE_CXX_STANDARD} # alternatively, compile the runtime with the same C++ standard as the outer project
+          -DCMAKE_CXX_STANDARD:STRING=${CMAKE_CXX_STANDARD}
       INSTALL_COMMAND ""
       EXCLUDE_FROM_ALL 1)
 endif()

third_party/protobuf.cmake

@@ -12,14 +12,16 @@ FetchContent_Declare(GTest
 )
 FetchContent_Declare(Protobuf
         GIT_REPOSITORY https://github.com/protocolbuffers/protobuf.git
-        GIT_TAG v28.2
+        GIT_TAG v29.3
         SYSTEM
         OVERRIDE_FIND_PACKAGE
 )
 
 # Disable warnings for dependency targets.
 set(protobuf_BUILD_TESTS OFF CACHE INTERNAL "")
 if(MSVC)
+  set(protobuf_MSVC_STATIC_RUNTIME OFF)
+  set(gtest_force_shared_crt ON)
   add_compile_options("/W0")
 else()
   add_compile_options("-w")