Skip to content

Adding dlib's fft dependency - #1054

Open
KV2773 wants to merge 5 commits into
microsoft:mainfrom
KV2773:general
Open

Adding dlib's fft dependency#1054
KV2773 wants to merge 5 commits into
microsoft:mainfrom
KV2773:general

Conversation

@KV2773

@KV2773 KV2773 commented Apr 24, 2026

Copy link
Copy Markdown

Using the build.sh as

./build.sh -DCMAKE_POLICY_VERSION_MINIMUM=3.5   -DONNXRUNTIME_LIB_DIR=/usr/local/lib -DONNXRUNTIME_INCLUDE_DIR=/usr/local/include/onnxruntime  -DCMAKE_INSTALL_PREFIX=/usr/local -DOCOS_ENABLE_OPENCV_CODECS=OFF -DOCOS_ENABLE_VISION=OFF -DOCOS_ENABLE_AUDIO=OFF -DOCOS_ENABLE_CV2=OFF

Problem:

[ 96%] Linking CXX shared library lib/libortextensions.so
ld: 0711-317 ERROR: Undefined symbol: ._ZN4dlib3fftIfEEvRKNS_8fft_sizeEPKNSt3__17complexIT_EEPS7_b
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
.ibm-clang: error: linker command failed with exit code 8 (use -v to see invocation)
gmake[2]: *** [CMakeFiles/extensions_shared.dir/build.make:109: lib/libortextensions.so] Error 8
gmake[1]: *** [CMakeFiles/Makefile2:639: CMakeFiles/extensions_shared.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....
/home/Kushal/onnxruntime-1.20.0/aixoss/onnxruntime-extensions/test/static_test/test_ustring.cc:32:29: warning: 'codecvt_utf8<char32_t>' is deprecated [-Wdeprecated-declarations]

After my changes.

[ 92%] Linking CXX executable bin/ocos_test
[100%] Built target ocos_test
[100%] Built target extensions_test

@KV2773
KV2773 requested a review from a team as a code owner April 24, 2026 14:10
@KV2773

KV2773 commented Apr 26, 2026

Copy link
Copy Markdown
Author

Hii @sayanshaw24 @skottmckay may you review it once.

@skottmckay

Copy link
Copy Markdown
Contributor

Does it need the functions in fft.cpp or just the definition of fft_size?

What code requires an fft_size definition and how is that dependency resolved with a default build?

@skottmckay

Copy link
Copy Markdown
Contributor

I would suggest adding something like this in the OCOS_ENABLE_MATH section before line 414 so that we only include the file when needed:

  # operators\math\energy_stft_segmentation.cc needs the dlib fft implementation
  list(APPEND TARGET_SRC ${TARGET_SRC_DLIB} ${dlib_SOURCE_DIR}/dlib/fft/fft.cpp)

list(APPEND TARGET_SRC ${TARGET_SRC_MATH} ${TARGET_SRC_DLIB} ${TARGET_SRC_INVERSE})

And now as multiple places could include fft.cpp we should remove duplicates at the end of configuration just before the add_library around line 5196 That's simpler than multiple places checking whether a file has been added previously or not.

add_library(noexcep_operators STATIC ${TARGET_SRC_NOEXCEPTION})
add_library(ocos_operators STATIC ${TARGET_SRC})

list(REMOVE_DUPLICATES TARGET_SRC_NOEXCEPTION)
list(REMOVE_DUPLICATES TARGET_SRC)

@KV2773

KV2773 commented Apr 28, 2026

Copy link
Copy Markdown
Author

Does it need the functions in fft.cpp or just the definition of fft_size?

What code requires an fft_size definition and how is that dependency resolved with a default build?

So it needs the definition of fft_size only as per my observation.

This function is used by these files

grep_64: out/AIX/RelWithDebInfo/CMakeFiles/ocos_operators.dir/operators/math/math.cc.o: binary file matches

So in a default build we are making the OCOS_ENABLE_AUDIO by default on and in which we are adding this file fft.cpp which will give this definition.

if(OCOS_ENABLE_AUDIO)
  if (NOT OCOS_ENABLE_DLIB)
    message(FATAL_ERROR "Audio operators require DLIB to be enabled.")
  endif()
  include(dr_libs)
  file(GLOB TARGET_SRC_AUDIO "operators/audio/*.*")
  list(APPEND TARGET_SRC_AUDIO ${dlib_SOURCE_DIR}/dlib/fft/fft.cpp)
  list(APPEND TARGET_SRC_NOEXCEPTION ${TARGET_SRC_AUDIO})
endif()

@KV2773

KV2773 commented Apr 29, 2026

Copy link
Copy Markdown
Author

I would suggest adding something like this in the OCOS_ENABLE_MATH section before line 414 so that we only include the file when needed:

  # operators\math\energy_stft_segmentation.cc needs the dlib fft implementation
  list(APPEND TARGET_SRC ${TARGET_SRC_DLIB} ${dlib_SOURCE_DIR}/dlib/fft/fft.cpp)

list(APPEND TARGET_SRC ${TARGET_SRC_MATH} ${TARGET_SRC_DLIB} ${TARGET_SRC_INVERSE})

And now as multiple places could include fft.cpp we should remove duplicates at the end of configuration just before the add_library around line 5196 That's simpler than multiple places checking whether a file has been added previously or not.

add_library(noexcep_operators STATIC ${TARGET_SRC_NOEXCEPTION})
add_library(ocos_operators STATIC ${TARGET_SRC})

list(REMOVE_DUPLICATES TARGET_SRC_NOEXCEPTION)
list(REMOVE_DUPLICATES TARGET_SRC)

Yep @skottmckay that way, also we can do, and your approach is better in that way.
Should I commit the changes or you will commit them.

Copilot AI review requested due to automatic review settings May 7, 2026 05:46
@KV2773

KV2773 commented May 7, 2026

Copy link
Copy Markdown
Author

Hii @sayanshaw24 may u review it once.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses an AIX link failure (Undefined symbol: dlib::fft<float>(...)) by ensuring dlib’s FFT implementation unit is compiled into the extensions build, even when audio operators are disabled.

Changes:

  • Adds dlib/fft/fft.cpp to the CMake source list for builds where math ops are enabled.
  • Minor whitespace cleanup around the ext_imgcodecs include.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread CMakeLists.txt Outdated
@KV2773

KV2773 commented May 8, 2026

Copy link
Copy Markdown
Author

Hii @skottmckay @sayanshaw24 may u review it once.

@KV2773

KV2773 commented May 18, 2026

Copy link
Copy Markdown
Author

Hii @skottmckay @sayanshaw24 , added the duplicity handling logic .
Can u review it once

@KV2773

KV2773 commented Jun 2, 2026

Copy link
Copy Markdown
Author

Hii @skottmckay @sayanshaw24 , can u review it once, to check if everything is right and then can merge it so that it doesn't fail on other systems.

KV2773 and others added 4 commits June 3, 2026 20:50
Copilot suggestion

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

Comment thread CMakeLists.txt
Comment on lines +413 to +415
if(NOT OCOS_ENABLE_AUDIO)
list(APPEND TARGET_SRC ${dlib_SOURCE_DIR}/dlib/fft/fft.cpp) # For the OCOS_ENABLE_AUDIO=OFF so that unknown symbol error shouldn't come.
endif()
Comment thread CMakeLists.txt
Comment on lines +514 to +516
#Removing duplicate files added to the target sources.
list(REMOVE_DUPLICATES TARGET_SRC_NOEXCEPTION)
list(REMOVE_DUPLICATES TARGET_SRC)
Comment thread CMakeLists.txt
endif()

if(NOT OCOS_ENABLE_AUDIO)
list(APPEND TARGET_SRC ${dlib_SOURCE_DIR}/dlib/fft/fft.cpp) # For the OCOS_ENABLE_AUDIO=OFF so that unknown symbol error shouldn't come.
@KV2773

KV2773 commented Jun 19, 2026

Copy link
Copy Markdown
Author

Heyy @skottmckay may you merge this PR if everything works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants