[libshortfin] Clean up a few CI related items. (#163)

* Disables ASAN ODR check properly (had syntax error in env var). * Builds non-debug Python. * Sets up LSAN suppressions to account for non-debug Python intentional leaks. * Has CMake fetch the correct nanobind vs relying on pip (had another incident of mismatched version). * Added options SHORTFIN_BUILD_STATIC=OFF, SHORTFIN_BUILD_DYNAMIC=ON, and SHORTFIN_LINK_DYNAMIC to control variants of libraries produced and linked to (disabling static in most configs saves a lot of build time). * Run pytest with `-s` (stream output) option. For some reason, some of the subprocess tests are otherwise not working right on GHA runners. It looks like there may be some malformed I/O somewhere that gets less reliable with redirection. This isn't solved but does increase the chance of passing.
nod-ai · Sep 3, 2024 · 949e43f · 949e43f
1 parent 08c69aa
commit 949e43f
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 78 deletions.
diff --git a/.github/workflows/ci_linux_x64-libshortfin.yml b/.github/workflows/ci_linux_x64-libshortfin.yml
@@ -94,6 +94,7 @@ jobs:
           -DCMAKE_C_COMPILER=clang-18 \
           -DCMAKE_CXX_COMPILER=clang++-18 \
           -DCMAKE_LINKER_TYPE=LLD \
+          -DSHORTFIN_BUNDLE_DEPS=ON \
           -DCMAKE_PREFIX_PATH=${{ env.IREE_REPO_DIR }}/build/lib/cmake/IREE \
           -DSHORTFIN_BUILD_PYTHON_BINDINGS=ON \
           ..
@@ -111,12 +112,16 @@ jobs:
       run: |
         mkdir ${{ env.LIBSHORTFIN_DIR }}/build-host-only
         cd ${{ env.LIBSHORTFIN_DIR }}/build-host-only
+        # In this configuration, also build static+dynamic in order to verify
+        # that path structurally works.
         cmake -GNinja \
           -DCMAKE_C_COMPILER=clang-18 \
           -DCMAKE_CXX_COMPILER=clang++-18 \
           -DCMAKE_LINKER_TYPE=LLD \
           -DCMAKE_PREFIX_PATH=${{ env.IREE_REPO_DIR }}/build/lib/cmake/IREE \
           -DSHORTFIN_BUILD_PYTHON_BINDINGS=ON \
           -DSHORTFIN_HAVE_AMDGPU=OFF \
+          -DSHORTFIN_BUILD_STATIC=ON \
+          -DSHORTFIN_BUILD_DYNAMIC=ON \
           ..
         cmake --build . --target all
diff --git a/.github/workflows/ci_linux_x64_asan-libshortfin.yml b/.github/workflows/ci_linux_x64_asan-libshortfin.yml
@@ -23,16 +23,19 @@ env:
   PYENV_ROOT: ${{ github.workspace }}/pyenv
   PYENV_REF: 9ecd803bffaffb949fbdd8c70cb086227f6a3202 # v2.4.10
   PYTHON_VER: 3.12.3
-  CACHE_ASAN_VER: 1
+  CACHE_ASAN_VER: 2
   CACHE_DEPS_VER: 1
   IREE_SOURCE_DIR: ${{ github.workspace }}/iree
   LIBSHORTFIN_DIR: ${{ github.workspace }}/libshortfin/
 
-
 jobs:
   setup-python-asan:
     name: Setup Python ASan
     runs-on: ubuntu-24.04
+    env:
+      # The Python build process leaks. Here we just disable leak checking vs
+      # being more precise.
+      ASAN_OPTIONS: detect_leaks=0
 
     steps:
     - name: Cache Python ASan
@@ -64,15 +67,18 @@ jobs:
         cd ${{ env.PYENV_ROOT }}
         src/configure && make -C src
         export PATH=${{ env.PYENV_ROOT }}/bin:$PATH && eval "$(pyenv init -)"
-        CC=clang-18 CXX=clang++-18 LDFLAGS="-lstdc++" PYTHON_CONFIGURE_OPTS="--with-address-sanitizer" pyenv install -v -g ${{ env.PYTHON_VER }}
-        pyenv global ${{ env.PYTHON_VER }}-debug
+        CC=clang-18 CXX=clang++-18 LDFLAGS="-lstdc++" PYTHON_CONFIGURE_OPTS="--with-address-sanitizer" pyenv install -v ${{ env.PYTHON_VER }}
+        pyenv global ${{ env.PYTHON_VER }}
 
 
   build-and-test:
     name: Build and test libshortfin
     needs: [setup-python-asan]
     runs-on: ubuntu-24.04
-
+    env:
+      # TODO(#151): Don't ignore ODR violations
+      ASAN_OPTIONS: detect_odr_violation=0
+      LSAN_OPTIONS: suppressions=${{ github.workspace }}/libshortfin/build_tools/python_lsan_suppressions.txt
     steps:
     - name: Install dependencies
       run: |
@@ -135,9 +141,6 @@ jobs:
         key: ${{ steps.cache-python-deps-restore.outputs.cache-primary-key }}
 
     - name: Build libshortfin
-      env:
-        # TODO(#151): Don't ignore ODR violations
-        ASAN_OPTIONS=detect_odr_violation: 0
       run: |
         eval "$(pyenv init -)"
         mkdir ${{ env.LIBSHORTFIN_DIR }}/build
@@ -168,4 +171,4 @@ jobs:
       run: |
         eval "$(pyenv init -)"
         cd ${{ env.LIBSHORTFIN_DIR }}
-        pytest -m "not requires_amd_gpu"
+        pytest -m "not requires_amd_gpu" -s
diff --git a/libshortfin/CMakeLists.txt b/libshortfin/CMakeLists.txt
@@ -18,6 +18,8 @@ project(
   VERSION 0.9
   LANGUAGES C CXX)
 
+include(CMakeDependentOption)
+
 set(SOVERSION 1)
 
 set(CMAKE_C_STANDARD 11)
@@ -34,10 +36,26 @@ endif()
 # build options
 option(SHORTFIN_BUILD_PYTHON_BINDINGS "Builds Python Bindings" OFF)
 option(SHORTFIN_BUILD_TESTS "Builds C++ tests" ON)
-option(SHORTFIN_BUNDLE_DEPS "Download dependencies instead of using system libraries" OFF)
+option(SHORTFIN_BUNDLE_DEPS "Download dependencies instead of using system libraries" ON)
 
 set(SHORTFIN_IREE_SOURCE_DIR "" CACHE FILEPATH "Path to IREE source")
 
+# Options for building static or dynamic libraries.
+option(SHORTFIN_BUILD_STATIC "Builds static libraries" OFF)
+option(SHORTFIN_BUILD_DYNAMIC "Builds dynamic libraries" ON)
+cmake_dependent_option(SHORTFIN_LINK_DYNAMIC "Links internal binaries against static libshortfin.a" ON "SHORTFIN_BUILD_DYNAMIC" OFF)
+if(NOT SHORTFIN_BUILD_STATIC AND NOT SHORTFIN_BUILD_DYNAMIC)
+  message(FATAL_ERROR "One of SHORTFIN_BUILD_STATIC or SHORTFIN_BUILD_DYNAMIC must be ON")
+endif()
+message(STATUS "Shortfin build static = ${SHORTFIN_BUILD_STATIC}, dynamic = ${SHORTFIN_BUILD_DYNAMIC}")
+if(SHORTFIN_LINK_DYNAMIC)
+  message(STATUS "Dynamic linking to shortfin")
+  set(SHORTFIN_LINK_LIBRARY_NAME "shortfin")
+else()
+  message(STATUS "Static linking to shortfin-static")
+  set(SHORTFIN_LINK_LIBRARY_NAME "shortfin-static")
+endif()
+
 # Enabling ASAN. Note that this will work best if building in a completely
 # bundled fashion and with an ASAN rigged CPython. Otherwise, various LD_PRELOAD
 # hacks are needed. This is merely a develope convenience: people are more

diff --git a/libshortfin/bindings/python/CMakeLists.txt b/libshortfin/bindings/python/CMakeLists.txt
@@ -10,12 +10,13 @@
 # build. - _shortfin_tracing.lib: Native library with tracing enabled (TODO). -
 # Others.
 
-# Detect the installed nanobind package and import it into CMake
-execute_process(
-  COMMAND "${Python_EXECUTABLE}" -m nanobind --cmake_dir
-  OUTPUT_STRIP_TRAILING_WHITESPACE
-  OUTPUT_VARIABLE nanobind_ROOT)
-find_package(nanobind CONFIG REQUIRED)
+# nanobind
+FetchContent_Declare(
+  nanobind
+  GIT_REPOSITORY https://github.com/wjakob/nanobind.git
+  GIT_TAG        9641bb7151f04120013b812789b3ebdfa7e7324f # 2.1.0
+)
+FetchContent_MakeAvailable(nanobind)
 
 nanobind_add_module(shortfin_python_extension NB_STATIC LTO
   array_binding.cc
@@ -26,9 +27,7 @@ set_target_properties(shortfin_python_extension
                       PROPERTIES OUTPUT_NAME "_shortfin_default/lib")
 
 target_link_libraries(shortfin_python_extension
-  # TODO: This should be configurable as to whether we link to the static
-  # or dynamic version.
-  PRIVATE shortfin
+  PRIVATE ${SHORTFIN_LINK_LIBRARY_NAME}
 )
 
 nanobind_add_stub(

diff --git a/libshortfin/build_tools/cmake/shortfin_library.cmake b/libshortfin/build_tools/cmake/shortfin_library.cmake
@@ -24,20 +24,24 @@ function(shortfin_public_library)
     "COMPONENTS"
     ${ARGN}
   )
-  shortfin_components_to_static_libs(_STATIC_COMPONENTS ${_RULE_COMPONENTS})
-  shortfin_components_to_dynamic_libs(_DYLIB_COMPONENTS ${_RULE_COMPONENTS})
-  # Static library.
-  add_library("${_RULE_NAME}-static" STATIC)
-  target_link_libraries(
-    "${_RULE_NAME}-static" PUBLIC ${_STATIC_COMPONENTS}
-  )
+  if(SHORTFIN_BUILD_STATIC)
+    # Static library.
+    shortfin_components_to_static_libs(_STATIC_COMPONENTS ${_RULE_COMPONENTS})
+    add_library("${_RULE_NAME}-static" STATIC)
+    target_link_libraries(
+      "${_RULE_NAME}-static" PUBLIC ${_STATIC_COMPONENTS}
+    )
+  endif()
 
-  # Dylib library.
-  add_library("${_RULE_NAME}" SHARED)
-  target_compile_definitions("${_RULE_NAME}" INTERFACE _SHORTFIN_USING_DYLIB)
-  target_link_libraries(
-    "${_RULE_NAME}" PUBLIC ${_DYLIB_COMPONENTS}
-  )
+  if(SHORTFIN_BUILD_DYNAMIC)
+    # Dylib library.
+    shortfin_components_to_dynamic_libs(_DYLIB_COMPONENTS ${_RULE_COMPONENTS})
+    add_library("${_RULE_NAME}" SHARED)
+    target_compile_definitions("${_RULE_NAME}" INTERFACE _SHORTFIN_USING_DYLIB)
+    target_link_libraries(
+      "${_RULE_NAME}" PUBLIC ${_DYLIB_COMPONENTS}
+    )
+  endif()
 endfunction()
 
 function(shortfin_cc_component)
@@ -48,50 +52,52 @@ function(shortfin_cc_component)
     "HDRS;SRCS;DEPS;COMPONENTS"
     ${ARGN}
   )
-  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
-  set(_STATIC_OBJECTS_NAME "${_RULE_NAME}.objects")
-  set(_DYLIB_OBJECTS_NAME "${_RULE_NAME}.dylib.objects")
-
-  shortfin_components_to_static_libs(_STATIC_COMPONENTS ${_RULE_COMPONENTS})
-  shortfin_components_to_dynamic_libs(_DYLIB_COMPONENTS ${_RULE_COMPONENTS})
-
-  # Static object library.
-  add_library(${_STATIC_OBJECTS_NAME} OBJECT)
-  target_sources(${_STATIC_OBJECTS_NAME}
-    PRIVATE
-      ${_RULE_SRCS}
-      ${_RULE_HDRS}
-  )
-  target_compile_options(${_STATIC_OBJECTS_NAME} PRIVATE ${SHORTFIN_DEFAULT_COPTS})
-  target_link_libraries(${_STATIC_OBJECTS_NAME}
-    PUBLIC
-      _shortfin_defs
-      ${_STATIC_COMPONENTS}
-      ${_RULE_DEPS}
-  )
+  if(SHORTFIN_BUILD_STATIC)
+    # Static object library.
+    set(_STATIC_OBJECTS_NAME "${_RULE_NAME}.objects")
+    shortfin_components_to_static_libs(_STATIC_COMPONENTS ${_RULE_COMPONENTS})
+    add_library(${_STATIC_OBJECTS_NAME} OBJECT)
+    target_sources(${_STATIC_OBJECTS_NAME}
+      PRIVATE
+        ${_RULE_SRCS}
+        ${_RULE_HDRS}
+    )
+    target_compile_options(${_STATIC_OBJECTS_NAME} PRIVATE ${SHORTFIN_DEFAULT_COPTS})
+    target_link_libraries(${_STATIC_OBJECTS_NAME}
+      PUBLIC
+        _shortfin_defs
+        ${_STATIC_COMPONENTS}
+        ${_RULE_DEPS}
+    )
+  endif()
 
-  # Dylib object library.
-  add_library(${_DYLIB_OBJECTS_NAME} OBJECT)
-  target_sources(${_DYLIB_OBJECTS_NAME}
-    PRIVATE
-      ${_RULE_SRCS}
-      ${_RULE_HDRS}
-  )
-  target_compile_options(${_DYLIB_OBJECTS_NAME} PRIVATE ${SHORTFIN_DEFAULT_COPTS})
-  target_link_libraries(${_DYLIB_OBJECTS_NAME}
-    PUBLIC
-      _shortfin_defs
-      ${_DYLIB_COMPONENTS}
-      ${_RULE_DEPS}
-  )
-  set_target_properties(
-    ${_DYLIB_OBJECTS_NAME} PROPERTIES
-    CXX_VISIBILITY_PRESET hidden
-    C_VISIBILITY_PRESET hidden
-    VISIBILITY_INLINES_HIDDEN ON
-  )
-  target_compile_definitions(${_DYLIB_OBJECTS_NAME}
-    PRIVATE _SHORTFIN_BUILDING_DYLIB)
+  if(SHORTFIN_BUILD_DYNAMIC)
+    set(CMAKE_POSITION_INDEPENDENT_CODE ON)
+    set(_DYLIB_OBJECTS_NAME "${_RULE_NAME}.dylib.objects")
+    shortfin_components_to_dynamic_libs(_DYLIB_COMPONENTS ${_RULE_COMPONENTS})
+    # Dylib object library.
+    add_library(${_DYLIB_OBJECTS_NAME} OBJECT)
+    target_sources(${_DYLIB_OBJECTS_NAME}
+      PRIVATE
+        ${_RULE_SRCS}
+        ${_RULE_HDRS}
+    )
+    target_compile_options(${_DYLIB_OBJECTS_NAME} PRIVATE ${SHORTFIN_DEFAULT_COPTS})
+    target_link_libraries(${_DYLIB_OBJECTS_NAME}
+      PUBLIC
+        _shortfin_defs
+        ${_DYLIB_COMPONENTS}
+        ${_RULE_DEPS}
+    )
+    set_target_properties(
+      ${_DYLIB_OBJECTS_NAME} PROPERTIES
+      CXX_VISIBILITY_PRESET hidden
+      C_VISIBILITY_PRESET hidden
+      VISIBILITY_INLINES_HIDDEN ON
+    )
+    target_compile_definitions(${_DYLIB_OBJECTS_NAME}
+      PRIVATE _SHORTFIN_BUILDING_DYLIB)
+  endif()
 endfunction()
 
 function(shortfin_components_to_static_libs out_static_libs)
@@ -122,7 +128,7 @@ function(shortfin_gtest_test)
   add_executable(${_RULE_NAME} ${_RULE_SRCS})
   target_link_libraries(${_RULE_NAME} PRIVATE
     ${_RULE_DEPS}
-    shortfin
+    ${SHORTFIN_LINK_LIBRARY_NAME}
     GTest::gmock
     GTest::gtest_main
   )

diff --git a/libshortfin/build_tools/python_lsan_suppressions.txt b/libshortfin/build_tools/python_lsan_suppressions.txt
@@ -0,0 +1,2 @@
+leak:PyUnicode_New
+leak:_PyUnicodeWriter_Finish
diff --git a/libshortfin/pyproject.toml b/libshortfin/pyproject.toml
@@ -3,7 +3,6 @@ requires = [
     "cmake>=3.29",
     "setuptools>=61.0",
     "wheel",
-    "nanobind>=2.0",
     "ninja",
 ]
 build-backend = "setuptools.build_meta"

diff --git a/libshortfin/requirements-tests.txt b/libshortfin/requirements-tests.txt
@@ -1,4 +1,3 @@
-nanobind==2.1.0
 pytest
 requests
 fastapi