Conversation
Patch libsodium for windows to create both debug and release lib files.
There was a problem hiding this comment.
Pull request overview
This PR removes reliance on LD_LIBRARY_PATH injection by switching to build-time RPATH handling (with an option to force absolute paths), updates CI to stop exporting LD_LIBRARY_PATH, and adds a new CTest fixture to validate RPATH/origin behavior. It also refactors the Windows/MSVC libsodium port to build and install both Debug and Release artifacts via MSBuild.
Changes:
- Stop modifying
LD_LIBRARY_PATHand instead populateCMAKE_BUILD_RPATH/CMAKE_HOST_BUILD_RPATH(defaulting to loader-relative origin paths unless explicitly configured otherwise). - Add an
RPATHtest fixture and wire it into the test suite on supported UNIX hosts. - Rework libsodium on MSVC to build Debug+Release outputs and import them as a proper multi-config imported target.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/rpath/CMakeLists.txt | New configure-time fixture to assert toolset RPATH/origin behavior and that LD_LIBRARY_PATH is not modified. |
| test/CMakeLists.txt | Adds CTest configure invocations for the new rpath fixture (UNIX, non-cross builds). |
| README.md | Documents ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_USE_ABSOLUTE_RPATH and clarifies LD_LIBRARY_PATH is not modified. |
| ports/ssl/libsodium/libsodium.cmake | Routes MSVC builds through the new MSVC helper and prefers locally built artifacts when present. |
| ports/ssl/libsodium/libsodium-msvc.cmake | New MSVC-specific build/import implementation that produces Debug+Release libs (and DLLs for shared). |
| ports/Configure.cmake | Introduces absolute-vs-origin build RPATH composition and removes prior LD_LIBRARY_PATH injection. |
| ci/do_ci.sh | Stops setting LD_LIBRARY_PATH in CI and unsets it up-front. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ${CMAKE_TOOLSET_RPATH_TEST_GENERATOR_OPTIONS} -DATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_USE_ABSOLUTE_RPATH=ON | ||
| -DCMAKE_BUILD_RPATH_USE_ORIGIN=ON -DCMAKE_HOST_BUILD_RPATH_USE_ORIGIN=ON | ||
| -DCMAKE_TOOLSET_RPATH_TEST_EXPECT_USE_ORIGIN=ON) | ||
| endif() |
Comment on lines
+245
to
+247
| cmake-toolset does not modify `LD_LIBRARY_PATH`. Additional build `RPATH` entries use `$ORIGIN` on Unix or | ||
| `@loader_path` on Apple by default; set `ATFRAMEWORK_CMAKE_TOOLSET_THIRD_PARTY_USE_ABSOLUTE_RPATH=ON` to use absolute | ||
| third-party prefix paths instead. Windows DLL lookup continues to use `PATH`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Patch libsodium for windows to create both debug and release lib files.