Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc(CMakeLists.txt): ENABLE_EXTERNAL_PLUGINS not compatible with Windows #968

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wzv5
Copy link

@wzv5 wzv5 commented Dec 28, 2024

Feature

fix Windows build when ENABLE_EXTERNAL_PLUGINS

Unit test

  • Done

Manual test

  • Done

Code Review

  1. Unit and manual test pass
  2. GitHub Action CI pass
  3. At least one contributor reviews and votes
  4. Can be merged clean without conflicts
  5. PR will be merged by rebase upstream base

Additional Info

@lotem
Copy link
Member

lotem commented Dec 29, 2024

How does it work?

@wzv5
Copy link
Author

wzv5 commented Dec 29, 2024

There is no libdl on Windows, and an error will occur when linking.

@wzv5
Copy link
Author

wzv5 commented Dec 29, 2024

Snipaste_2024-12-29_18-42-00

删掉dl依赖后,可成功编译,并且运行正常。

@lotem
Copy link
Member

lotem commented Dec 30, 2024

我持懷疑態度。請多測試測試。
舊有的經驗是跨鏈接模塊的對象析構會有問題。
因爲採用了靜態鏈接C++運行時庫,在堆上分配內存的對象應該回到分配內存的模塊里執行析構函數,但 librime 的代碼沒有這樣的保證。

@wzv5
Copy link
Author

wzv5 commented Dec 30, 2024

哦,我理解您说的问题,但该pr仅用于修复windows系统上的链接错误,并不涉及内存分配,至少可以让windows系统与linux、macos的功能一致。
至于您说的跨模块的对象析构问题可能需要其他更复杂的pr来解决。

@lotem
Copy link
Member

lotem commented Dec 30, 2024

哦,我理解您说的问题,但该pr仅用于修复windows系统上的链接错误,并不涉及内存分配,至少可以让windows系统与linux、macos的功能一致。 至于您说的跨模块的对象析构问题可能需要其他更复杂的pr来解决。

開了這個選項,插件 DLL 基本上無法使用,比如現在常用那些插件,就完全沒信心不會造成崩潰。
解決方案:要麼將編譯選項改爲 /MD 並在分發產品時一併安裝VC運行時庫,要麼就得在 librime 代碼裏加大對內存對象所有權的控制,現在大量對象用 shared_ptr 包裝,不知道會在哪裏釋放。
這些問題不解決,就無法在 Windows 上動態加載插件,功能一致就無從談起。

那麼問題來了,既然無法工作,爲什麼要在 Windows 上打開這個 CMake 選項呢?

@wzv5 wzv5 changed the title fix Windows build when ENABLE_EXTERNAL_PLUGINS doc(CMakeLists.txt): ENABLE_EXTERNAL_PLUGINS not compatible with Windows Dec 30, 2024
@wzv5
Copy link
Author

wzv5 commented Dec 30, 2024

我回退了修改,并且修改了CMakeLists.txt里的说明,以免其他人像我一样折腾半天。

@lotem
Copy link
Member

lotem commented Dec 30, 2024

其實你倒是提醒了我,如若動態加載插件的功能有非常大的需求,用家爲此可以接受安裝額外的系統組件,那麼動態鏈接微軟的 VC++ 運行時庫不失爲一個可行的技術方案。基本不需要 C++ 代碼的更動,但是要加一整套構建、發佈的流程,最後做成一個獨立的發行版。想必要投入的工夫也不會少,你如果有興趣可以繼續研究。

@wzv5
Copy link
Author

wzv5 commented Dec 30, 2024

我会继续研究看看,不过我的想法是优化librime导出接口,避免直接导出C++类,这样也便于其他语言实现插件。

@wzv5
Copy link
Author

wzv5 commented Dec 31, 2024

@lotem 已按你说的将整个项目改为/MD,并且把所有插件都编译为dll,成功跑起来了,暂时没发现问题。
如果你觉得这样可行,我就把代码推上来。
不过weasel项目没有使用cmake,不太方便切换编译配置,具体编译方式还需要琢磨一下。

I20241231 22:27:24.870422 17604 plugins_module.cc:40] loading plugins from rime-plugins
I20241231 22:27:24.870551 17604 plugins_module.cc:49] loading plugin 'lua' from rime-plugins\rime-lua.dll
I20241231 22:27:24.880337 17604 registry.cc:14] registering component: lua_translator
I20241231 22:27:24.880345 17604 registry.cc:14] registering component: lua_filter
I20241231 22:27:24.880348 17604 registry.cc:14] registering component: lua_segmentor
I20241231 22:27:24.880350 17604 registry.cc:14] registering component: lua_processor
I20241231 22:27:24.880351 17604 plugins_module.cc:62] loaded plugin: lua
I20241231 22:27:24.880401 17604 plugins_module.cc:49] loading plugin 'octagram' from rime-plugins\rime-octagram.dll
I20241231 22:27:24.882263 17604 registry.cc:14] registering component: grammar
I20241231 22:27:24.882269 17604 plugins_module.cc:62] loaded plugin: octagram
I20241231 22:27:24.882320 17604 plugins_module.cc:49] loading plugin 'predict' from rime-plugins\rime-predict.dll
I20241231 22:27:24.884150 17604 registry.cc:14] registering component: predictor
I20241231 22:27:24.884157 17604 registry.cc:14] registering component: predict_translator
I20241231 22:27:24.884160 17604 plugins_module.cc:62] loaded plugin: predict

@wzv5
Copy link
Author

wzv5 commented Jan 1, 2025

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