Conversation
owent
commented
Apr 8, 2026
owent
commented
- Removed obsolete libsodium-config.cmake.in file.
- Updated libsodium.cmake to improve target detection and handling.
- Enhanced the build process for both MSVC and Unix-like systems.
- Updated version from 1.0.20 to 1.0.21.
- Adjusted port.cmake to streamline libsodium linking.
- Removed obsolete libsodium-config.cmake.in file. - Updated libsodium.cmake to improve target detection and handling. - Enhanced the build process for both MSVC and Unix-like systems. - Updated version from 1.0.20 to 1.0.21. - Adjusted port.cmake to streamline libsodium linking.
There was a problem hiding this comment.
Pull request overview
This PR refactors how libsodium is discovered/built/linked in the SSL port layer, removes the previous CMake-based “build-script” integration, adds a new Findsodium.cmake module, and updates the bundled libsodium version pin to 1.0.21-RELEASE.
Changes:
- Update libsodium port logic (target detection + new MSVC/MSBuild vs Unix/autotools build flow) and bump version to 1.0.21.
- Replace the previous
FindLibsodium.cmake+find_package(Libsodium)approach with a newmodules/Findsodium.cmake+find_package(sodium)flow. - Simplify libsodium linkage aggregation in
ports/ssl/port.cmaketo prefer the exported port link target when present.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ports/ssl/port.cmake | Switch libsodium discovery/linking to exported link name or find_package(sodium). |
| ports/ssl/libsodium/libsodium.cmake | Refactor libsodium import/build logic; add MSBuild/autotools build paths; bump version to 1.0.21. |
| ports/ssl/libsodium/build-script/libsodium-config.cmake.in | Removed obsolete libsodium config template. |
| ports/ssl/libsodium/build-script/CMakeLists.txt | Removed large, custom CMake-based libsodium build script. |
| modules/Findsodium.cmake | New find-module for libsodium that provides an imported sodium target. |
| modules/FindLibsodium.cmake | Removed previous Libsodium find-module. |
| if(ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_USE_SHARED OR BUILD_SHARED_LIBS) | ||
| set(ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_MSBUILD_LINKAGE "DLL") | ||
| else() | ||
| set(ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_MSBUILD_LINKAGE "LIB") | ||
| endif() |
There was a problem hiding this comment.
The shared/static decision is computed as ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_USE_SHARED OR BUILD_SHARED_LIBS, which bypasses the toolset’s shared/static resolution priority (in particular, it ignores the per-port ..._USE_STATIC override when BUILD_SHARED_LIBS is ON). Please use project_third_party_check_build_shared_lib("libsodium", "", <out_var>) (or an equivalent helper) and branch on the resolved boolean instead, so ..._USE_STATIC reliably wins over BUILD_SHARED_LIBS.
| if(ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_PATCH_FILE | ||
| AND EXISTS "${ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_PATCH_FILE}") | ||
| list(APPEND ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_BUILD_OPTIONS GIT_PATCH_FILES | ||
| "${ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_PATCH_FILE}") | ||
| endif() |
There was a problem hiding this comment.
In the autotools branch, patch handling is appended to ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_BUILD_OPTIONS via GIT_PATCH_FILES, but that variable is never passed into the subsequent find_configure_package(...) invocation. As a result, any detected libsodium patch file will not actually be applied on Unix/MinGW. Pass GIT_PATCH_FILES "${ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_PATCH_FILE}" directly to find_configure_package(...) (or clone/apply patches explicitly) and remove the unused list mutation.
| if(NOT DEFINED sodium_USE_STATIC_LIBS) | ||
| option(sodium_USE_STATIC_LIBS "enable to statically link against sodium" OFF) | ||
| endif() | ||
| if(NOT (sodium_USE_STATIC_LIBS EQUAL sodium_USE_STATIC_LIBS_LAST)) |
There was a problem hiding this comment.
This comparison uses the numeric operator EQUAL on sodium_USE_STATIC_LIBS/sodium_USE_STATIC_LIBS_LAST, but these are typically boolean strings (ON/OFF). With non-numeric values, EQUAL can produce a configure-time error. Use STREQUAL (or normalize to 0/1 first) when detecting changes to the option.
| if(NOT (sodium_USE_STATIC_LIBS EQUAL sodium_USE_STATIC_LIBS_LAST)) | |
| if(NOT ("${sodium_USE_STATIC_LIBS}" STREQUAL "${sodium_USE_STATIC_LIBS_LAST}")) |
| # communicate results | ||
| include(FindPackageHandleStandardArgs) | ||
| find_package_handle_standard_args(sodium | ||
| REQUIRED_VARS | ||
| sodium_LIBRARY_RELEASE | ||
| sodium_LIBRARY_DEBUG | ||
| sodium_INCLUDE_DIR | ||
| VERSION_VAR | ||
| sodium_VERSION_STRING) |
There was a problem hiding this comment.
find_package_handle_standard_args() currently requires both sodium_LIBRARY_RELEASE and sodium_LIBRARY_DEBUG. This will fail on platforms/installs that only ship a single config (common on Unix, and also common for prebuilt Windows packages that only provide Release). Consider requiring only sodium_INCLUDE_DIR and at least one of the library variables, then falling back (e.g., set DEBUG=RELEASE when DEBUG is missing) so sodium_FOUND can be true in single-config setups.
| # create imported target | ||
| if(sodium_USE_STATIC_LIBS) | ||
| set(_LIB_TYPE STATIC) | ||
| else() | ||
| set(_LIB_TYPE SHARED) | ||
| endif() | ||
| add_library(sodium ${_LIB_TYPE} IMPORTED) | ||
|
|
There was a problem hiding this comment.
The module unconditionally calls add_library(sodium ... IMPORTED) when sodium_FOUND is true. If find_package(sodium) is invoked more than once in the same configure (which can happen because multiple ports call it), this will error because the target already exists. Please guard with if(TARGET sodium) return() (or skip target creation) before calling add_library.
| project_third_party_append_build_shared_lib_var( | ||
| "libsodium" "" ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_BUILD_OPTIONS BUILD_SHARED_LIBS) | ||
|
|
||
| if(ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_USE_SHARED OR BUILD_SHARED_LIBS) | ||
| list(APPEND ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_CONFIGURE_OPTIONS "--enable-shared" | ||
| "--disable-static") | ||
| else() | ||
| list(APPEND ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_CONFIGURE_OPTIONS "--disable-shared" | ||
| "--enable-static") | ||
| endif() |
There was a problem hiding this comment.
Same shared/static issue exists in the autotools branch: if(ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_LIBSODIUM_USE_SHARED OR BUILD_SHARED_LIBS) ignores the per-port ..._USE_STATIC override when BUILD_SHARED_LIBS is ON. Please branch on the resolved result from project_third_party_check_build_shared_lib("libsodium", "", <out_var>) (and use that same boolean consistently for --enable/disable-shared and for the export.h patch condition).
