Skip to content

Commit

Permalink
Revert Python finding logic and some CMake fixes for Windows (onnx#6382)
Browse files Browse the repository at this point in the history 
### Description
<!-- - Describe your changes. -->
Revert the python finding logic introduced by onnx#6381 due to unable to
handle Conda builds. However, the Python executable detection was
improved. In addition, some Windows warnings were re-enabled, and the
hidden visibility of onnx_cpp2py_export was fixed.
### Motivation and Context
For better code.
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->

---------

Signed-off-by: cyy <[email protected]>
  • Loading branch information
@cyyever
cyyever authored Sep 25, 2024
1 parent b196423 commit d14634e
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 29 deletions.
47 changes: 19 additions & 28 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,16 @@ if(ONNX_DISABLE_EXCEPTIONS)
endif()
endif()

if(DEFINED PYTHON_EXECUTABLE)
message(STATUS "Use Python_EXECUTABLE ${Python_EXECUTABLE}")
endif()

# find_package Python has replaced PythonInterp and PythonLibs since cmake 3.12
# Use the following command in the future; now this is only compatible with the latest pybind11
# find_package(Python ${PY_VERSION} COMPONENTS Interpreter Development REQUIRED)
find_package(PythonInterp ${PY_VERSION} REQUIRED)
if(BUILD_ONNX_PYTHON)
find_package(Python ${PY_VERSION} COMPONENTS Interpreter Development REQUIRED)
else()
find_package(Python ${PY_VERSION} COMPONENTS Interpreter REQUIRED)
find_package(PythonLibs ${PY_VERSION})
endif()

if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
Expand Down Expand Up @@ -358,7 +364,7 @@ function(RELATIVE_PROTOBUF_GENERATE_CPP NAME SRCS HDRS ROOT_DIR DEPEND)
endif()

add_custom_command(OUTPUT "${GENERATED_PROTO}"
COMMAND Python::Interpreter "${GEN_PROTO_PY}"
COMMAND "${PYTHON_EXECUTABLE}" "${GEN_PROTO_PY}"
ARGS ${GEN_PROTO_ARGS}
DEPENDS ${INFILE}
COMMENT "Running gen_proto.py on ${INFILE}"
Expand Down Expand Up @@ -541,8 +547,8 @@ if(BUILD_ONNX_PYTHON)

add_library(onnx_cpp2py_export MODULE "${ONNX_ROOT}/onnx/cpp2py_export.cc")
set_target_properties(onnx_cpp2py_export PROPERTIES PREFIX "")
set_target_properties(onnx_cpp2py_export
PROPERTIES COMPILE_FLAGS "-fvisibility=hidden")
set_target_properties(onnx_cpp2py_export PROPERTIES CXX_VISIBILITY_PRESET hidden)
set_target_properties(onnx_cpp2py_export PROPERTIES VISIBILITY_INLINES_HIDDEN 1)
set_target_properties(onnx_cpp2py_export PROPERTIES SUFFIX ${PY_EXT_SUFFIX})
set_target_properties(onnx_cpp2py_export
PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR})
Expand All @@ -563,7 +569,7 @@ if(BUILD_ONNX_PYTHON)

target_include_directories(onnx_cpp2py_export PUBLIC
"${pybind11_INCLUDE_DIRS}"
"${PYTHON_INCLUDE_DIRS}")
"${PYTHON_INCLUDE_DIR}")

if(APPLE)
set_target_properties(onnx_cpp2py_export
Expand Down Expand Up @@ -600,20 +606,14 @@ if(BUILD_ONNX_PYTHON)
target_link_libraries(onnx_cpp2py_export PRIVATE ${PYTHON_LIBRARIES})
target_compile_options(onnx_cpp2py_export
PRIVATE /MP
/wd4146 # unary minus operator applied to unsigned type,
# result still unsigned
/wd4244 # 'argument': conversion from 'google::
# protobuf::uint64' to 'int', possible
# loss of data
/wd4267 # Conversion from 'size_t' to 'int',
# possible loss of data
/wd4996 # The second parameter is ignored.
${EXTRA_FLAGS})
if(ONNX_USE_PROTOBUF_SHARED_LIBS)
target_compile_options(onnx_cpp2py_export
PRIVATE /wd4251 # 'identifier' : class 'type1' needs to
# have dll-interface to be used by
# clients of class 'type2'
)
endif()
add_msvc_runtime_flag(onnx_cpp2py_export)
add_onnx_global_defines(onnx_cpp2py_export)
endif()
Expand All @@ -629,6 +629,8 @@ endif()
if(MSVC)
target_compile_options(onnx_proto
PRIVATE /MP
/wd4146 # unary minus operator applied to unsigned type,
# result still unsigned
/wd4244 #'argument': conversion from 'google::
#protobuf::uint64' to 'int', possible
# loss of data
Expand All @@ -637,25 +639,14 @@ if(MSVC)
${EXTRA_FLAGS})
target_compile_options(onnx
PRIVATE /MP
/wd4146 # unary minus operator applied to unsigned type,
# result still unsigned
/wd4244 # 'argument': conversion from 'google::
# protobuf::uint64' to 'int', possible
# loss of data
/wd4267 # Conversion from 'size_t' to 'int',
# possible loss of data
/wd4996 # The second parameter is ignored.
${EXTRA_FLAGS})
if(ONNX_USE_PROTOBUF_SHARED_LIBS)
target_compile_options(onnx_proto
PRIVATE /wd4251 # 'identifier' : class 'type1' needs to
# have dll-interface to be used by
# clients of class 'type2'
)
target_compile_options(onnx
PRIVATE /wd4251 # 'identifier' : class 'type1' needs to
# have dll-interface to be used by
# clients of class 'type2'
)
endif()
add_msvc_runtime_flag(onnx_proto)
add_msvc_runtime_flag(onnx)
set(onnx_static_library_flags
Expand Down
20 changes: 19 additions & 1 deletion setup.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,24 @@ def get_ext_suffix():
return sysconfig.get_config_var("EXT_SUFFIX")


def get_python_execute():
if WINDOWS:
return sys.executable
# Try to search more accurate path, because sys.executable may return a wrong one,
# as discussed in https://github.com/python/cpython/issues/84399
python_dir = os.path.abspath(
os.path.join(sysconfig.get_path("include"), "..", "..")
)
if os.path.isdir(python_dir):
python_bin = os.path.join(python_dir, "bin", "python3")
if os.path.isfile(python_bin):
return python_bin
python_bin = os.path.join(python_dir, "bin", "python")
if os.path.isfile(python_bin):
return python_bin
return sys.executable


################################################################################
# Customized commands
################################################################################
Expand Down Expand Up @@ -159,7 +177,7 @@ def run(self):
cmake_args = [
CMAKE,
f"-DPYTHON_INCLUDE_DIR={sysconfig.get_path('include')}",
f"-DPYTHON_EXECUTABLE={sys.executable}",
f"-DPYTHON_EXECUTABLE={get_python_execute()}",
"-DBUILD_ONNX_PYTHON=ON",
"-DCMAKE_EXPORT_COMPILE_COMMANDS=ON",
f"-DONNX_NAMESPACE={ONNX_NAMESPACE}",
Expand Down

0 comments on commit d14634e

Please sign in to comment.