Skip to content

chore(utils): add portable to_chars_double fallback for macOS#72

Open
rayandrew wants to merge 1 commit into
llnl:developfrom
rayandrew:chore/version-0.0.8
Open

chore(utils): add portable to_chars_double fallback for macOS#72
rayandrew wants to merge 1 commit into
llnl:developfrom
rayandrew:chore/version-0.0.8

Conversation

@rayandrew
Copy link
Copy Markdown
Collaborator

  • Introduce to_chars_double wrapper that falls back to snprintf on macOS < 13.3
  • Force CPM-built zstd on Apple to avoid deployment target mismatches
  • Update version patch to 8

Copilot AI review requested due to automatic review settings May 23, 2026 05:14
Copy link
Copy Markdown

Copilot AI left a comment

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 improves macOS portability by avoiding unsupported std::to_chars(double) on older Apple deployment targets and by ensuring zstd is built in a way that matches the wheel’s deployment target (avoiding host-library mismatches during wheel repair).

Changes:

  • Add a dftracer::utils::to_chars_double wrapper with an Apple fallback to snprintf for macOS deployment targets < 13.3, and use it in bloom_visitor.cpp.
  • On Apple, pin CMAKE_OSX_DEPLOYMENT_TARGET from MACOSX_DEPLOYMENT_TARGET in the CMake cache and prefer CPM-built zstd over system zstd.
  • Bump project version patch from 7 to 8.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/dftracer/utils/utilities/composites/dft/visitors/bloom_visitor.cpp Switch double formatting to a portable to_chars_double wrapper.
include/dftracer/utils/core/common/to_chars.h New portable double-to-string helper with macOS fallback.
CMakeLists.txt Cache-declare CMAKE_OSX_DEPLOYMENT_TARGET from environment on Apple; bump patch version.
cmake/modules/Dependencies.cmake Avoid system zstd on Apple and pass deployment target into CPM zstd build.
Comments suppressed due to low confidence (1)

cmake/modules/Dependencies.cmake:1405

  • On Apple, find_package(zstd ...) is skipped, but zstd_FOUND can still be TRUE from a previous configure (it’s also written to the cache later in this function). In that case this if(zstd_FOUND) branch will run and can silently select a cached/system zstd, defeating the "always CPM-build on Apple" intent. Consider explicitly forcing zstd_FOUND to FALSE (and clearing cached include/lib hints) when APPLE, or changing the condition to only honor zstd_FOUND when NOT APPLE.
  if(NOT APPLE)
    find_package(zstd QUIET CONFIG)
    if(NOT zstd_FOUND)
      find_path(zstd_INCLUDE_DIRS NAMES zstd.h)
      find_library(zstd_LIBRARIES NAMES zstd)
      if(zstd_INCLUDE_DIRS AND zstd_LIBRARIES)
        set(zstd_FOUND TRUE)
      endif()
    endif()
  endif()

  if(zstd_FOUND)
    message(STATUS "Found system zstd")
    if(DEFINED zstd_LIBRARIES)

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

Comment thread cmake/modules/Dependencies.cmake Outdated
Comment thread include/dftracer/utils/core/common/to_chars.h
@rayandrew rayandrew force-pushed the chore/version-0.0.8 branch from 808e67e to 2db6929 Compare May 23, 2026 05:51
…ate zstd handling

- Introduce to_chars_double wrapper that falls back to snprintf on macOS < 13.3
- Force CPM-built zstd on Apple to avoid deployment target mismatches
- Update version patch to 8
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.

2 participants