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.

Sign up for GitHub

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

Es0m/add static runtime linking #469

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Es0m/add static runtime linking #469

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9ee52c
setting msvc runtime and compiler flag if OCOS_ENABLE_STATIC_LIB is set.
es0m-msft Jun 11, 2023
6f2ef40
only set build type if on win32
es0m-msft Jun 11, 2023
d777b5b
added check for RelWithDebInfo and moved parameter check below option…
es0m-msft Jun 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.25)
project(onnxruntime_extensions LANGUAGES C CXX)

# set(CMAKE_VERBOSE_MAKEFILE ON)
if(NOT CMAKE_BUILD_TYPE)
if(NOT WIN32 AND NOT CMAKE_BUILD_TYPE)
es0m marked this conversation as resolved.
Show resolved Hide resolved
message(STATUS "Build type not set - using RelWithDebInfo")
set(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "Choose build type: Debug Release RelWithDebInfo." FORCE)
endif()
Expand Down Expand Up @@ -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>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can set the CMAKE_MSVC_RUNTIME_LIBRARY variable from cmake command line. So this change is unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCOS_STATIC_LIBRARY is different from OCOS_ENABLE_STATIC_LIBRARY?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snnn unfortunately, you can't if you want to respect generator expressions (I tried!)
Would you like a different OPTION for this usecase?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif ()

check_cxx_compiler_flag(-sdl HAS_SDL)
check_cxx_compiler_flag(-Qspectre HAS_QSPECTRE)
if (HAS_QSPECTRE)
Expand Down