Es0m/add static runtime linking #469
Conversation
es0m
commented
Jun 11, 2023
es0m
commented
- current main of onnxruntime-extensions depends on msvc runtime (vcruntime)
- static linkage is desired to reduce downstream dependency
- OCOS_ENABLE_STATIC_LIB is an existing option which only seems to be used for webassembly checks atm
- added add_compile_options(/MT$<$CONFIG:Debug:d>) and CMAKE_MSVC_RUNTIME_LIBRARY for static linkage if OCOS_ENABLE_STATIC_LIB is specified
CMakeLists.txt
Outdated
| message(STATUS "Build type not set - using RelWithDebInfo") | ||
| set(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "Choose build type: Debug Release RelWithDebInfo." FORCE) | ||
| endif() | ||
| if (OCOS_ENABLE_STATIC_LIB) |
|
/azp run extensions.ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… definition and into MSVC block
|
/azp run extensions.ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can you add the failed build case that you are fixing in ort-extensions CI pipeline to make sure the static link build won't be broken by any future change? onnxruntime-extensions/.pipelines/ci.yml Line 260 in 1f0c76c @snnn , can you review this PR as well? |
| @@ -105,6 +105,11 @@ if(NOT CC_OPTIMIZE) | |||
| endif() | |||
|
|
|||
| if (MSVC) | |||
| if (OCOS_ENABLE_STATIC_LIB) | |||
| add_compile_options(/MT$<$<OR:$<CONFIG:Debug>,$<CONFIG:RelWithDebInfo>>:d>) | |||
| set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<OR:$<CONFIG:Debug>,$<CONFIG:RelWithDebInfo>>:Debug>") | |||
There was a problem hiding this comment.
You can set the CMAKE_MSVC_RUNTIME_LIBRARY variable from cmake command line. So this change is unnecessary.
There was a problem hiding this comment.
OCOS_ENABLE_STATIC_LIB means we want to build this repo's code as a static library. However, it doesn't mean they need to static link to VC Runtime.
There was a problem hiding this comment.
@es0m-msft, According to the description, this PR is to enable static linking to VCRuntime, but OCOS_STATIC_LIBRARY flag was created for generating the static library of ort-extensions.lib instead of DLL. Can you try @snnn suggestion to see if it meets your requirements?
There was a problem hiding this comment.
OCOS_STATIC_LIBRARY is different from OCOS_ENABLE_STATIC_LIBRARY?
There was a problem hiding this comment.
@snnn unfortunately, you can't if you want to respect generator expressions (I tried!)
Would you like a different OPTION for this usecase?
There was a problem hiding this comment.
I searched the code, the only occurrence of "OCOS_ENABLE_STATIC_LIB" is:
if(NOT OCOS_ENABLE_STATIC_LIB AND CMAKE_SYSTEM_NAME STREQUAL "Emscripten")So, it has nothing to do with VC Runtime. OCOS_ENABLE_STATIC_LIB has effects only when CMAKE_SYSTEM_NAME equals to "Emscripten", which does not use vc runtime at all. So I don't think the variable is used to control which VC Runtime to use. And we should not give the variable a new meaning(which will surprise the person who created this cmake option).
There was a problem hiding this comment.
ONNX Runtime's build.py has a build option: --enable_msvc_static_runtime. It works by setting CMAKE_MSVC_RUNTIME_LIBRARY cmake variable to MultiThreaded$<$<CONFIG:Debug>:Debug. If you build this repo with the main ONNX Runtime repo, you can use that build option. Otherwise you can do the same thing when you invoke cmake. Therefore I think there is no need to introduce a new cmake variable to control this. Every cmake project should respect the CMAKE_MSVC_RUNTIME_LIBRARY cmake variable's value.
There was a problem hiding this comment.
And, as CMAKE_MSVC_RUNTIME_LIBRARY means "the default value of MSVC_RUNTIME_LIBRARY". In cmake files, we should not set CMAKE_MSVC_RUNTIME_LIBRARY as it will change the default value instead of the actual value: MSVC_RUNTIME_LIBRARY.
There was a problem hiding this comment.
https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html#variable:CMAKE_MSVC_RUNTIME_LIBRARY
If you follow the docs, you're supposed to set CMAKE_MSVC_RUNTIME_LIBRARY not MSVC_RUNTIME_LIBRARY.
There was a problem hiding this comment.
regarding your use of OCOS_ENABLE_STATIC_LIB -- I'd suggest writing some documentation on how to use the flag. It is unclear as you can see by it's usage. It's related to WASM only apparently.
Also, instead of having some command line collection parameters assembled as per build.py, having a clear option to enable the required functionality might be better.
|
Hi @wenbingl let me check the output on my end. I believe more needs to be set (pthreads_enable_static, etc.) |
