Skip to content

Commit

Permalink
More windows-related nits
Browse files Browse the repository at this point in the history
- use `!` instead of a `not` macro
- Don't build the `planparser` tool on windows; this uses unix commands to parse arguments.
- std::filesystem::path doesn't implicitly convert to std::string on msvc
- A shared library must specify export symbols (or set `CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS`) for a .lib file to be generated, which is required for things to link against said library.
- Can't use `*` (wildcard) on windows in a CMake command; use built-in CMake globbing instead.
  • Loading branch information
mortbopet committed Oct 10, 2024
1 parent 68036ca commit aa44efc
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 10 deletions.
1 change: 1 addition & 0 deletions export/planloader/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: Apache-2.0

set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
add_library(planloader SHARED planloader.cpp)

add_dependencies(planloader substrait_io)
Expand Down
8 changes: 6 additions & 2 deletions src/substrait/common/tests/IoTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
#include <gmock/gmock-matchers.h>
#include <gtest/gtest.h>
#include <protobuf-matchers/protocol-buffer-matchers.h>

#ifndef _WIN32
#include <unistd.h>
#endif

using ::protobuf_matchers::EqualsProto;
using ::protobuf_matchers::Partially;
Expand Down Expand Up @@ -45,8 +48,9 @@ TEST_F(IoTest, LoadMissingFile) {
class SaveAndLoadTestFixture : public ::testing::TestWithParam<PlanFileFormat> {
public:
void SetUp() override {
testFileDirectory_ = std::filesystem::temp_directory_path() /
std::filesystem::path("my_temp_dir");
testFileDirectory_ = (std::filesystem::temp_directory_path() /
std::filesystem::path("my_temp_dir"))
.string();

if (!std::filesystem::create_directory(testFileDirectory_)) {
ASSERT_TRUE(false) << "Failed to create temporary directory.";
Expand Down
2 changes: 1 addition & 1 deletion src/substrait/textplan/converter/ReferenceNormalizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ bool compareExtensionFunctions(
return make_tuple(
// First sort so that extension functions precede any other kind of
// extension.
not decl.has_extension_function(),
!decl.has_extension_function(),
// Next sort by space.
decl.extension_function().extension_uri_reference(),
// Finally sort by name within a space.
Expand Down
10 changes: 6 additions & 4 deletions src/substrait/textplan/parser/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,14 @@ target_link_libraries(
absl::status
absl::statusor)

add_executable(planparser Tool.cpp)

target_link_libraries(planparser substrait_textplan_loader error_listener)
if(UNIX)
add_executable(planparser Tool.cpp)
target_link_libraries(planparser substrait_textplan_loader error_listener)
install(TARGETS planparser EXPORT SubstraitTargets)
endif()
install(TARGETS substrait_textplan_loader EXPORT SubstraitTargets)

if(${SUBSTRAIT_CPP_BUILD_TESTING})
add_subdirectory(tests)
endif()

install(TARGETS planparser substrait_textplan_loader EXPORT SubstraitTargets)
16 changes: 14 additions & 2 deletions src/substrait/textplan/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ add_test_case(

cmake_path(GET CMAKE_CURRENT_SOURCE_DIR PARENT_PATH TEXTPLAN_SOURCE_DIR)

# Get all JSON files in the data directory
file(GLOB JSON_FILES "${TEXTPLAN_SOURCE_DIR}/data/*.json")

add_custom_command(
TARGET substrait_textplan_round_trip_test
POST_BUILD
Expand All @@ -49,8 +52,17 @@ add_custom_command(
${CMAKE_COMMAND} -E copy
"${TEXTPLAN_SOURCE_DIR}/converter/data/q6_first_stage.json"
"${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json"
COMMAND ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/data/*.json"
"${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/")
)

foreach(json_file ${TEXTPLAN_JSON_FILES})
add_custom_command(
TARGET substrait_textplan_round_trip_test
POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy
"${json_file}"
"${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/"
)
endforeach()

message(
STATUS "test data will be here: ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data"
Expand Down
2 changes: 1 addition & 1 deletion src/substrait/textplan/tests/RoundtripTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ std::vector<std::string> getTestCases() {
testDataPath.append("data");
for (auto const& dirEntry :
std::filesystem::recursive_directory_iterator{testDataPath}) {
std::string pathName = dirEntry.path();
std::string pathName = dirEntry.path().string();
if (endsWith(pathName, ".json") &&
!endsWith(pathName, "q6_first_stage.json")) {
filenames.push_back(pathName);
Expand Down

0 comments on commit aa44efc

Please sign in to comment.