fix: install DLL to bin directory on Windows - #408
Merged
Merged
Conversation
Contributor
|
Hi @kt286. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Contributor
|
Warning
详情 {
"export": {
"dtkgui.cmake": {
"a": [
" set(CMAKE_SHARED_LINKER_FLAGS \"${CMAKE_SHARED_LINKER_FLAGS} -Wl,--export-all-symbols\")"
]
}
}
} |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates Windows symbol export handling and fixes DLL installation paths by replacing a global linker flag hack with proper CMake target properties and by splitting install destinations per component so Windows runtime binaries land in the correct directory. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider scoping
WINDOWS_EXPORT_ALL_SYMBOLSunder aWIN32condition so the intent that this is a Windows-specific behavior is clearer to future readers, even if it is currently harmless on other platforms. - You moved
LIBDTKGUI_LIBRARYfrom a globaladd_definitionstotarget_compile_definitionson${LIB_NAME}; double-check whether any other targets in this project relied on that definition and, if so, set it on those specific targets as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `WINDOWS_EXPORT_ALL_SYMBOLS` under a `WIN32` condition so the intent that this is a Windows-specific behavior is clearer to future readers, even if it is currently harmless on other platforms.
- You moved `LIBDTKGUI_LIBRARY` from a global `add_definitions` to `target_compile_definitions` on `${LIB_NAME}`; double-check whether any other targets in this project relied on that definition and, if so, set it on those specific targets as well.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kt286
force-pushed
the
fix/windows-cmake
branch
from
August 20, 2026 06:12
b0c11b6 to
2ede452
Compare
Contributor
|
Warning
详情 {
"export": {
"dtkgui.cmake": {
"a": [
" set(CMAKE_SHARED_LINKER_FLAGS \"${CMAKE_SHARED_LINKER_FLAGS} -Wl,--export-all-symbols\")"
]
}
}
} |
kt286
marked this pull request as draft
August 20, 2026 06:16
kt286
force-pushed
the
fix/windows-cmake
branch
2 times, most recently
from
August 20, 2026 12:53
0aee093 to
fc0bcee
Compare
kt286
marked this pull request as ready for review
August 20, 2026 12:55
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The description mentions switching to
WINDOWS_EXPORT_ALL_SYMBOLS, but the code change still relies on the globalCMAKE_SHARED_LINKER_FLAGSwith-Wl,--export-all-symbols; consider using the target propertyWINDOWS_EXPORT_ALL_SYMBOLSon the relevant targets instead to align with the stated intent and avoid global flag pollution. - The
if(WIN32)block now applies-Wl,--export-all-symbolseven when using MSVC, which does not understand-Wloptions; you may want to keep theNOT MSVCguard or conditionally apply this only for MinGW-like toolchains.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The description mentions switching to `WINDOWS_EXPORT_ALL_SYMBOLS`, but the code change still relies on the global `CMAKE_SHARED_LINKER_FLAGS` with `-Wl,--export-all-symbols`; consider using the target property `WINDOWS_EXPORT_ALL_SYMBOLS` on the relevant targets instead to align with the stated intent and avoid global flag pollution.
- The `if(WIN32)` block now applies `-Wl,--export-all-symbols` even when using MSVC, which does not understand `-Wl` options; you may want to keep the `NOT MSVC` guard or conditionally apply this only for MinGW-like toolchains.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Separate install destinations for LIBRARY, ARCHIVE and RUNTIME to ensure DLL files are installed to the bin directory on Windows instead of lib. fix: Windows 平台 DLL 安装到 bin 目录 分离 LIBRARY、ARCHIVE 和 RUNTIME 的安装路径，确保 Windows 平台的 DLL 文件安装到 bin 目录而非 lib 目录。
kt286
force-pushed
the
fix/windows-cmake
branch
from
August 21, 2026 06:16
fc0bcee to
03de8de
Compare
kt286
changed the title
BLumia
approved these changes
Aug 21, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, kt286 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
3 participants
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.
Replace compiler-specific -Wl,--export-all-symbols linker flag with the cmake target property WINDOWS_EXPORT_ALL_SYMBOLS ON. This is the recommended cross-platform approach and avoids polluting global linker flags.
Also split the install(TARGETS) command into per-component destinations (LIBRARY, ARCHIVE, RUNTIME) so that DLL files are installed to the bin directory on Windows, which is required by the Windows runtime loader.
fix: 在 Windows 上使用 WINDOWS_EXPORT_ALL_SYMBOLS 并修复 DLL 安装路径
用 CMake target 属性 WINDOWS_EXPORT_ALL_SYMBOLS ON 替代编译器特定的 -Wl,--export-all-symbols 链接器标志，这是推荐的跨平台做法，避免污染
全局链接器标志。
同时将 install(TARGETS) 拆分为按组件指定安装目录（LIBRARY、ARCHIVE、 RUNTIME），使 DLL 文件在 Windows 上安装到 bin 目录，符合 Windows 运行时 加载器的要求。
Summary by Sourcery
Fix Windows symbol exports and ensure runtime libraries are installed in the correct directory.
Bug Fixes:
Enhancements: