Skip to content

Do not simply rely on the compiler to check if we should link with the math library - #93

Merged
marton78 merged 2 commits into
marton78:masterfrom
G4m4:cmake_math_lib
Apr 13, 2026
Merged

Do not simply rely on the compiler to check if we should link with the math library#93
marton78 merged 2 commits into
marton78:masterfrom
G4m4:cmake_math_lib

Conversation

@G4m4

@G4m4 G4m4 commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Fix #92: first look for the math library using standard CMake, then only link with it explicitly if not compiling for Windows nor Mac.

@marton78 marton78 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Two blocking issues found:


Critical: MATHLIB uninitialized when PFFFT_DISABLE_LINK_WITH_M=ON on Unix

In the new code, when MATH_LIBRARY AND NOT WIN32 AND NOT APPLE is true but PFFFT_DISABLE_LINK_WITH_M is set, MATHLIB is never assigned. The opt-out branch needs an explicit set(MATHLIB "").

Suggested fix:

if(PFFFT_DISABLE_LINK_WITH_M)
  set(MATHLIB "")  # ← this line is missing
  message(STATUS "INFO: PFFFT_DISABLE_LINK_WITH_M set: will not link math lib m")
else()

Important: CI failures on build_win_mingw and build_win_msvc

The most likely cause is the change from set(MATHLIB "m") to set(MATHLIB ${MATH_LIBRARY}) — on Unix, find_library returns an absolute path (e.g. /usr/lib/libm.so), not just m. Passing an absolute path to target_link_libraries changes linker behavior and can break cross-compilation toolchains. The Windows CI failures need to be diagnosed with this in mind.


Minor issues

  • find_library(MATH_LIBRARY m) runs unconditionally on all platforms including Windows, where it always returns NOTFOUND — better to guard with if(NOT WIN32 AND NOT APPLE)
  • Missing status message when the opt-out flag is active (it was present in the original code)

The overall architecture is sound — using find_library plus platform guards is the right approach. These are fixable issues.

@marton78
marton78 merged commit 8c9c7db into marton78:master Apr 13, 2026
5 of 7 checks passed
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.

Build fails when using clang on Windows

2 participants