Merge pull request #2826 from tomjakubowski/sdist-ci-followups

texodus · web-flow · commit b9af3bdc9e82 · 2024-11-06T15:53:42.000-05:00
CI sdist fixes, port conda patch, fix ODR violation
diff --git a/.github/actions/install-deps/action.yaml b/.github/actions/install-deps/action.yaml
@@ -42,11 +42,6 @@ inputs:
         default: "false"
         description: "Don't use cache from previous builds"
 
-outputs:
-    VCPKG_INSTALLATION_ROOT:
-        description: "The output of vcpkg install path"
-        value: ${{ steps.vcpkg-step.outputs.VCPKG_INSTALLATION_ROOT }}
-
 runs:
     using: "composite"
     steps:
@@ -111,23 +106,6 @@ runs:
         #   with:
         #       key: ${{ github.job }}-${{ matrix.os }}
 
-        - name: Windows init steps (vc143)
-          id: vcpkg-step
-          shell: pwsh
-          run: |
-              vcpkg.exe install boost-thread boost-algorithm boost-filesystem boost-multi-index boost-multiprecision boost-program-options boost-system boost-unordered boost-uuid
-              vcpkg.exe integrate install
-              echo "VCPKG_INSTALLATION_ROOT=${env:VCPKG_INSTALLATION_ROOT}"
-              echo "VCPKG_INSTALLATION_ROOT=${env:VCPKG_INSTALLATION_ROOT}" >> $env:GITHUB_OUTPUT
-              echo "${env:VCPKG_INSTALLATION_ROOT}" >> $env:GITHUB_PATH
-              echo "VCPKG_ROOT=${env:VCPKG_INSTALLATION_ROOT}" >> $env:GITHUB_ENV
-              dir env:
-          env:
-              PYTHON_VERSION: ${{ matrix.python-version }}
-              VCPKG_DEFAULT_TRIPLET: x64-windows
-              VCPKG_PLATFORM_TOOLSET: v143
-          if: ${{ runner.os == 'Windows' && inputs.cpp == 'true' }}
-
         # https://github.com/apache/arrow/issues/38391
         - if: ${{ runner.os == 'macOS' }}
           shell: bash
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -649,6 +649,8 @@ jobs:
     test_python_sdist:
         needs: [build_and_test_jupyterlab]
         runs-on: ${{ matrix.os }}
+        if: ${{ startsWith(github.ref, 'refs/tags/v') }}
+
         strategy:
             fail-fast: false
             matrix:
@@ -677,28 +679,36 @@ jobs:
 
             - uses: actions/download-artifact@v4
               with:
-                  name: perspective-metadata
-                  path: rust/
+                  name: perspective-rust
+                  path: crates/
 
             - uses: actions/download-artifact@v4
               with:
                   name: perspective-python-sdist
 
-            - name: Set up global perspective-server and perspective-client path overrides
-              # this makes it so the sdist is tested against unreleased changes
-              # made to perspective-client and perspective-server
+            # The sdist depends on perspective-{client,server} crates published to
+            # crates.io.  Patch the build to instead use .crate artifacts built
+            # previously in the workflow
+            - name: Set up path overrides for perspective .crate contents
+              shell: bash
               run: |
+                  (cd crates && tar xvf perspective-client-*.crate && tar xvf perspective-server-*.crate)
+                  CLIENT_CRATE_ROOT=$(ls -d "$PWD"/crates/perspective-client-*/ | head -n 1)
+                  SERVER_CRATE_ROOT=$(ls -d "$PWD"/crates/perspective-server-*/ | head -n 1)
+
                   mkdir -p $HOME/.cargo
                   cat > $HOME/.cargo/config.toml <<EOF
                   [patch.crates-io]
-                  perspective-client = { path = "$PWD/rust/perspective-client" }
-                  perspective-server = { path = "$PWD/rust/perspective-server" }
+                  perspective-client = { path = "$CLIENT_CRATE_ROOT" }
+                  perspective-server = { path = "$SERVER_CRATE_ROOT" }
                   EOF
 
             - name: Install perspective-python sdist
+              shell: bash
               run: python -m pip install -vv ./perspective*.tar.gz
 
             - name: Verify labextension
+              shell: bash
               run: |
                   jupyter labextension list --debug
                   jupyter labextension list 2>&1 | grep '@finos/perspective-jupyterlab'
@@ -830,6 +840,7 @@ jobs:
             [
                 build_and_test_jupyterlab,
                 test_python,
+                test_python_sdist,
                 test_js,
                 benchmark_js,
                 benchmark_python,
diff --git a/cmake/arrow.txt.in b/cmake/arrow.txt.in
@@ -20,5 +20,5 @@ ExternalProject_Add(apachearrow
   TEST_COMMAND      ""
   # This patch is to work around https://github.com/apache/arrow/issues/44384
   # It can be removed when a version of Arrow is released with https://github.com/apache/arrow/pull/44385
-  PATCH_COMMAND     ${CMAKE_COMMAND} -E chdir <SOURCE_DIR> git apply ${CMAKE_SOURCE_DIR}/patches/fix_arrow_libtool.patch
+  PATCH_COMMAND     "${CMAKE_COMMAND}" -E chdir <SOURCE_DIR> git apply "${CMAKE_SOURCE_DIR}/patches/fix_arrow_libtool.patch"
 )
diff --git a/cpp/perspective/CMakeLists.txt b/cpp/perspective/CMakeLists.txt
@@ -50,7 +50,7 @@ endif()
 
 
 if(WIN32)
-    set(CMAKE_CXX_FLAGS " /EHsc /MP /MT /c /bigobj")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /EHsc /MP /MT /c /bigobj")
 else()
     # set(CMAKE_CXX_FLAGS " ${CMAKE_CXX_FLAGS}")
 endif()
@@ -301,24 +301,24 @@ elseif(PSP_CPP_BUILD OR PSP_PYTHON_BUILD)
     endif()
 endif()
 
-set(RAPIDJSON_BUILD_TESTS OFF CACHE BOOL "Disable rapidjson tests")
-
-
-set(CMAKE_C_FLAGS "  \
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} \
 -O3 \
 ")
+
 if (PSP_WASM_EXCEPTIONS)
-set(CMAKE_CXX_FLAGS " -fwasm-exceptions \
--O3 \
--g0 \
-")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} \
+    -fwasm-exceptions \
+    -O3 \
+    -g0 \
+    ")
 else()
-set(CMAKE_CXX_FLAGS " \
--O3 \
-")
-
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} \
+    -O3 \
+    ")
 endif()
 
+set(RAPIDJSON_BUILD_TESTS OFF CACHE BOOL "Disable rapidjson tests")
+
 if(PSP_PYODIDE)
     set(RELOCATABLE_FLAGS "-sRELOCATABLE=1 -sSIDE_MODULE=2 -sWASM_BIGINT=1")
 
@@ -327,7 +327,7 @@ if(PSP_PYODIDE)
     string(APPEND CMAKE_CXX_FLAGS "${RELOCATABLE_FLAGS}")
 endif()
 
-# Build header-only dependencies from external source
+# Build (read: download and extract) header-only dependencies from external sources
 set(all_deps_INCLUDE_DIRS "")
 psp_build_dep("date" "${PSP_CMAKE_MODULE_PATH}/date.txt.in")
 psp_build_dep("hopscotch" "${PSP_CMAKE_MODULE_PATH}/hopscotch.txt.in")
@@ -342,6 +342,10 @@ list(APPEND all_deps_INCLUDE_DIRS
 psp_build_dep("Boost" "${PSP_CMAKE_MODULE_PATH}/Boost.txt.in")
 list(APPEND all_deps_INCLUDE_DIRS ${Boost_INCLUDE_DIRS})
 
+# Build dependencies as static libraries with add_subdirectory()
+# Note that values set above for CMAKE_C_FLAGS, CMAKE_CXX_FLAGS, etc. will
+# apply to dependency cmake builds
+
 # Arrow builds its own dependencies
 psp_build_message("${Cyan}Building Apache Arrow${ColorReset}")
 psp_build_dep("arrow" "${PSP_CMAKE_MODULE_PATH}/arrow.txt.in")
@@ -364,20 +368,16 @@ list(APPEND all_deps_INCLUDE_DIRS
 add_subdirectory(${PSP_CMAKE_MODULE_PATH}/../cpp/protos "${CMAKE_BINARY_DIR}/protos-build")
 
 # ####################
-set(CMAKE_C_FLAGS_DEBUG "")
-set(CMAKE_C_FLAGS_RELEASE "")
 set(CMAKE_C_FLAGS " \
     ${CMAKE_C_FLAGS} \
+    ${CMAKE_C_FLAGS_RELEASE} \
     ${EXTENDED_FLAGS} \
     ${OPT_FLAGS} \
     ")
 
-# prevents the default debug flags from overriding the debug flags we
-# set in OPT_FLAGS
-set(CMAKE_CXX_FLAGS_DEBUG "")
-set(CMAKE_CXX_FLAGS_RELEASE "")
 set(CMAKE_CXX_FLAGS " \
     ${CMAKE_CXX_FLAGS} \
+    ${CMAKE_CXX_FLAGS_RELEASE} \
     ${EXTENDED_FLAGS} \
     ${OPT_FLAGS} \
     ")
@@ -484,7 +484,7 @@ set(WASM_SOURCE_FILES ${SOURCE_FILES})
 message("${BUILD_MESSAGE}\n")
 
 if(WIN32)
-    set(CMAKE_CXX_FLAGS " /EHsc /MP /MT /c /bigobj")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /EHsc /MP /MT /c /bigobj")
 else()
     # set(CMAKE_CXX_FLAGS " ${CMAKE_CXX_FLAGS}")
 endif()
diff --git a/cpp/perspective/src/cpp/utils.cpp b/cpp/perspective/src/cpp/utils.cpp
@@ -125,12 +125,6 @@ parse_all_date_time(
     );
 }
 
-bool
-parse_all_date_time(std::tm& tm, std::string_view date_time_str) {
-    std::chrono::system_clock::time_point tp;
-    return parse_all_date_time(tm, tp, date_time_str);
-}
-
 bool
 parse_all_date_time(
     std::chrono::system_clock::time_point& tp, std::string_view date_time_str
diff --git a/rust/perspective-server/build/psp.rs b/rust/perspective-server/build/psp.rs
@@ -58,26 +58,6 @@ pub fn cmake_build() -> Result<Option<PathBuf>, std::io::Error> {
         dst.define("PSP_WASM_BUILD", "0");
     }
 
-    if cfg!(windows) {
-        match std::env::var("VCPKG_ROOT") {
-            Ok(vcpkg_root) => {
-                dst.define(
-                    "CMAKE_TOOLCHAIN_FILE",
-                    format!(
-                        "{}/scripts/buildsystems/vcpkg.cmake",
-                        vcpkg_root.replace("\\", "/")
-                    ),
-                );
-            },
-            Err(_) => {
-                println!(
-                    "cargo:warning=MESSAGE VCPKG_ROOT not set in environment, not setting \
-                     CMAKE_TOOLCHAIN_FILE"
-                )
-            },
-        }
-    }
-
     if std::env::var("CARGO_FEATURE_PYTHON").is_ok() {
         dst.define("CMAKE_POSITION_INDEPENDENT_CODE", "ON");
         dst.define("PSP_PYTHON_BUILD", "1");

Original file line number	Diff line number	Diff line change
`@@ -20,5 +20,5 @@ ExternalProject_Add(apachearrow`
`20`	`20`	`TEST_COMMAND ""`
`21`	`21`	`# This patch is to work around https://github.com/apache/arrow/issues/44384`
`22`	`22`	`# It can be removed when a version of Arrow is released with https://github.com/apache/arrow/pull/44385`
`23`		`- PATCH_COMMAND ${CMAKE_COMMAND} -E chdir <SOURCE_DIR> git apply ${CMAKE_SOURCE_DIR}/patches/fix_arrow_libtool.patch`
	`23`	`+ PATCH_COMMAND "${CMAKE_COMMAND}" -E chdir <SOURCE_DIR> git apply "${CMAKE_SOURCE_DIR}/patches/fix_arrow_libtool.patch"`
`24`	`24`	`)`