diff --git a/.github/workflows/build_test.yml b/.github/workflows/build_test.yml index 7b4f3574..686e9135 100644 --- a/.github/workflows/build_test.yml +++ b/.github/workflows/build_test.yml @@ -1,5 +1,5 @@ # SPDX-License-Identifier: Apache-2.0 -name: Ubuntu Build & Test +name: Build & Test on: push: @@ -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 diff --git a/CMakeLists.txt b/CMakeLists.txt index 9fae3ba9..70e89002 100644 --- a/CMakeLists.txt +++ b/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}) diff --git a/export/planloader/tests/CMakeLists.txt b/export/planloader/tests/CMakeLists.txt index 4115612b..b44dbdf8 100644 --- a/export/planloader/tests/CMakeLists.txt +++ b/export/planloader/tests/CMakeLists.txt @@ -32,3 +32,13 @@ add_custom_command( message( STATUS "test data will be here: ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data" ) + +# For Windows: copy planloader dll to the test executable directory so that it +# can be found during test execution. +if(WIN32) + add_custom_command( + TARGET planloader_test + POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy $ + "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests") +endif() diff --git a/scripts/find_vs.ps1 b/scripts/find_vs.ps1 new file mode 100644 index 00000000..0751bf12 --- /dev/null +++ b/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 diff --git a/src/substrait/common/tests/IoTest.cpp b/src/substrait/common/tests/IoTest.cpp index e97ea063..e3bd33a4 100644 --- a/src/substrait/common/tests/IoTest.cpp +++ b/src/substrait/common/tests/IoTest.cpp @@ -52,7 +52,8 @@ class SaveAndLoadTestFixture : public ::testing::TestWithParam { 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(); } @@ -86,7 +87,6 @@ TEST_P(SaveAndLoadTestFixture, SaveAndLoad) { read->mutable_named_table()->add_names("table_name"); auto status = ::io::substrait::savePlan(plan, tempFilename, encoding); ASSERT_TRUE(status.ok()) << "Save failed.\n" << status; - auto result = ::io::substrait::loadPlan(tempFilename); ASSERT_TRUE(result.ok()) << "Load failed.\n" << result.status(); ASSERT_THAT( @@ -109,14 +109,24 @@ TEST_P(SaveAndLoadTestFixture, SaveAndLoad) { })"))); } +static auto getFormats() { + return testing::Values( + PlanFileFormat::kBinary, + PlanFileFormat::kJson, + PlanFileFormat::kProtoText + +#ifndef _WIN32 + // Text format is currently not supported on Windows + , + PlanFileFormat::kText +#endif + ); +} + INSTANTIATE_TEST_SUITE_P( SaveAndLoadTests, SaveAndLoadTestFixture, - testing::Values( - PlanFileFormat::kBinary, - PlanFileFormat::kJson, - PlanFileFormat::kProtoText, - PlanFileFormat::kText), + getFormats(), [](const testing::TestParamInfo& info) { return planFileEncodingToString(info.param); }); diff --git a/src/substrait/function/tests/FunctionLookupTest.cpp b/src/substrait/function/tests/FunctionLookupTest.cpp index 8bbff887..5460cdb0 100644 --- a/src/substrait/function/tests/FunctionLookupTest.cpp +++ b/src/substrait/function/tests/FunctionLookupTest.cpp @@ -2,6 +2,7 @@ #include +#include #include #include "substrait/function/FunctionLookup.h" @@ -12,9 +13,10 @@ class FunctionLookupTest : public ::testing::Test { protected: static std::string getExtensionAbsolutePath() { const std::string absolutePath = __FILE__; - auto const pos = absolutePath.find_last_of('/'); - return absolutePath.substr(0, pos) + - "/../../../../third_party/substrait/extensions/"; + std::filesystem::path path(absolutePath); + std::filesystem::path parentDir = path.parent_path(); + return (parentDir / "../../../../third_party/substrait/extensions/") + .string(); } void SetUp() override { diff --git a/src/substrait/textplan/converter/LoadBinary.cpp b/src/substrait/textplan/converter/LoadBinary.cpp index 6691b507..f1d8696b 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -43,8 +43,8 @@ class StringErrorCollector : public google::protobuf::io::ErrorCollector { } // namespace absl::StatusOr readFromFile(std::string_view msgPath) { - std::ifstream textFile(std::string{msgPath}); - if (textFile.fail()) { + std::ifstream file(std::string{msgPath}, std::ios::binary); + if (file.fail()) { auto currDir = std::filesystem::current_path().string(); return absl::ErrnoToStatus( errno, @@ -52,7 +52,7 @@ absl::StatusOr readFromFile(std::string_view msgPath) { "Failed to open file {} when running in {}", msgPath, currDir)); } std::stringstream buffer; - buffer << textFile.rdbuf(); + buffer << file.rdbuf(); return buffer.str(); } diff --git a/src/substrait/textplan/converter/SaveBinary.cpp b/src/substrait/textplan/converter/SaveBinary.cpp index 165ef534..f5603e9f 100644 --- a/src/substrait/textplan/converter/SaveBinary.cpp +++ b/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(); } diff --git a/src/substrait/textplan/parser/tests/TextPlanParserTest.cpp b/src/substrait/textplan/parser/tests/TextPlanParserTest.cpp index 461dbd64..ecbfdf72 100644 --- a/src/substrait/textplan/parser/tests/TextPlanParserTest.cpp +++ b/src/substrait/textplan/parser/tests/TextPlanParserTest.cpp @@ -27,7 +27,14 @@ class TestCase { ::testing::Matcher expectedMatch; }; -class TextPlanParserTestFixture : public ::testing::TestWithParam {}; +class TextPlanParserTestFixture : public ::testing::TestWithParam { + void SetUp() override { +#if defined(_WIN32) || defined(_WIN64) + GTEST_SKIP() << "Skipping textplanparser test on Windows."; +#endif + ::testing::TestWithParam::SetUp(); + } +}; std::vector getTestCases() { static std::vector cases = { @@ -1301,7 +1308,7 @@ std::vector getTestCases() { return cases; } -TEST(TextPlanParser, LoadFromFile) { +TEST_P(TextPlanParserTestFixture, LoadFromFile) { auto stream = loadTextFile("data/provided_sample1.splan"); ASSERT_TRUE(stream.has_value()) << "Test input file missing."; auto result = parseStream(&*stream); diff --git a/src/substrait/textplan/tests/ParseResultMatchers.cpp b/src/substrait/textplan/tests/ParseResultMatchers.cpp index 5801ae2b..7fe12bfd 100644 --- a/src/substrait/textplan/tests/ParseResultMatchers.cpp +++ b/src/substrait/textplan/tests/ParseResultMatchers.cpp @@ -149,16 +149,20 @@ 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 extraSymbols; + auto expectedSymbolsSorted = expectedSymbols_; + std::sort(expectedSymbolsSorted.begin(), expectedSymbolsSorted.end()); if (listener != nullptr) { - std::vector 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) { @@ -166,14 +170,13 @@ class HasSymbolsMatcher { } } - std::vector missingSymbols(expectedSymbols_.size()); + std::vector 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 { diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index f3247590..6e1f74ea 100644 --- a/third_party/CMakeLists.txt +++ b/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 $ + $ + $ + $ + $) + 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) diff --git a/third_party/antlr4/cmake/ExternalAntlr4Cpp.cmake b/third_party/antlr4/cmake/ExternalAntlr4Cpp.cmake index 19ac84ce..d5724b32 100644 --- a/third_party/antlr4/cmake/ExternalAntlr4Cpp.cmake +++ b/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() diff --git a/third_party/protobuf.cmake b/third_party/protobuf.cmake index 4cf786f3..a8b6ff7d 100644 --- a/third_party/protobuf.cmake +++ b/third_party/protobuf.cmake @@ -12,7 +12,7 @@ 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 ) @@ -20,6 +20,8 @@ FetchContent_Declare(Protobuf # 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")