Fix add_definitions(-DWITH_SERVER) to use target_compile_definitions in three CMakeLists.txt files#5036
add_definitions(-DWITH_SERVER) to use target_compile_definitions in three CMakeLists.txt files#5036Conversation
add_definitions(-DWITH_SERVER) to use target_compile_definitions in three CMakeLists.txt files
|
@copilot all builds are failing here, whereas they succeeded in the base branch. |
Fixed in the latest commit. The |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## sftp-session-type #5036 +/- ##
==================================================
Coverage 87.35% 87.35%
==================================================
Files 277 277
Lines 14706 14706
==================================================
Hits 12845 12845
Misses 1861 1861 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8f43290 to
f61e025
Compare
|
Adopted into base branch. |
Description
Replace directory-scoped
add_definitions(-DWITH_SERVER)with target-scopedtarget_compile_definitions()in three CMakeLists.txt files to prevent theWITH_SERVERdefinition from leaking into unrelated targets.Changes Made
src/ssh/CMakeLists.txt: Replacedadd_definitions(-DWITH_SERVER)(insideadd_ssh_target()) withtarget_compile_definitions(${TARGET_NAME} PRIVATE WITH_SERVER), placed after theadd_library()call. This prevents the definition from leaking into later targets such assftp_clientandssh_client.src/sshfs_mount/CMakeLists.txt: Same fix insideadd_sshfs_mount_target().tests/unit/CMakeLists.txt: Replaced the directory-leveladd_definitions(-DWITH_SERVER)withtarget_compile_definitions(multipass_cpp_tests PRIVATE WITH_SERVER), scoping it to only the test executable that requires it.These changes address the feedback from #5029.
Related Issue(s)
Testing
No new unit tests required; this is a CMake scoping fix with no change to compiled code logic.
Manual testing steps:
WITH_SERVERdefined where expected.sftp_clientandssh_clientno longer receive theWITH_SERVERdefinition.Screenshots (if applicable)
Checklist
Additional Notes
Original PR: #5029
Triggering review: #5029 (comment)