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

Jump to bottom

[onnx] update ONNXIFI build output and host dependencies #20112

Closed
wants to merge 8 commits into from
Closed

[onnx] update ONNXIFI build output and host dependencies #20112

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8ee205b
[onnx] feature: foxi
luncliff Sep 11, 2021
d42ca2f
[onnx] force onnx/onnx_proto static in Windows
luncliff Sep 16, 2021
5114b48
[onnx] support windows static triplets
luncliff Sep 16, 2021
28cec17
[onnx] fix wrong LICENSE install
luncliff Sep 16, 2021
f8ec3f5
[onnx] remove feature 'foxi'
luncliff Sep 18, 2021
18d4965
Merge branch 'master' of https://github.com/microsoft/vcpkg into port…
Sep 23, 2021
233b72e
[onnx] back to ONLY_STATIC_LIBRARY
luncliff Sep 28, 2021
94525df
[onnx] fix auto-formatted version JSON
luncliff Sep 28, 2021
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
75 changes: 75 additions & 0 deletions ports/onnx/fix-cmakelists.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,78 @@ index 628dcaa..300e4ea 100644
install(DIRECTORY ${ONNX_ROOT}/onnx
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
FILES_MATCHING

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 300e4ea..7773f7a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -356,7 +356,13 @@ list(REMOVE_ITEM __tmp_srcs "${ONNX_ROOT}/onnx/cpp2py_export.cc")
list(REMOVE_ITEM __tmp_srcs ${onnx_gtests_src})
list(APPEND ONNX_SRCS ${__tmp_srcs})

-add_library(onnx_proto ${ONNX_PROTO_SRCS} ${ONNX_PROTO_HDRS})
+if(WIN32)
+ # see .azure-pipelines/Windwos-CI.yml
+ add_library(onnx_proto STATIC ${ONNX_PROTO_SRCS} ${ONNX_PROTO_HDRS})
+else()
+ add_library(onnx_proto ${ONNX_PROTO_SRCS} ${ONNX_PROTO_HDRS})
+endif()
JackBoosY marked this conversation as resolved.
Show resolved Hide resolved
+
target_include_directories(onnx_proto PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
$<INSTALL_INTERFACE:include>
@@ -410,6 +416,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
# whole-archive linker option not available on AIX.
# So, create a object library
add_library(onnx OBJECT ${ONNX_SRCS})
+elseif(WIN32)
+ add_library(onnx STATIC ${ONNX_SRCS}) # see .azure-pipelines/Windwos-CI.yml
JackBoosY marked this conversation as resolved.
Show resolved Hide resolved
else()
add_library(onnx ${ONNX_SRCS})
endif()
@@ -738,11 +746,17 @@ install(EXPORT ONNXTargets DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ONNX")
install(TARGETS
onnx onnx_proto
onnxifi onnxifi_dummy onnxifi_loader
- EXPORT ONNXTargets DESTINATION ${CMAKE_INSTALL_LIBDIR})
+ EXPORT ONNXTargets
+ RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+ LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+ ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})

if(NOT ANDROID AND NOT IOS)
install(TARGETS onnxifi_wrapper
- EXPORT ONNXTargets DESTINATION lib)
+ EXPORT ONNXTargets
+ RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+ LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+ ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
endif()

if(ONNXIFI_DUMMY_BACKEND)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index ac584a5..9f94181 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -647,7 +647,7 @@ endif()

if (NOT ANDROID AND NOT IOS)
# ---[ ONNXIFI wrapper
- add_library(onnxifi_wrapper MODULE onnx/onnxifi_wrapper.c)
+ add_library(onnxifi_wrapper onnx/onnxifi_wrapper.c)
JackBoosY marked this conversation as resolved.
Show resolved Hide resolved
if(MSVC)
add_msvc_runtime_flag(onnxifi_wrapper)
endif()
@@ -677,7 +677,11 @@ if (NOT ANDROID AND NOT IOS)
endif()

# ---[ ONNXIFI dummy backend
-add_library(onnxifi_dummy SHARED onnx/onnxifi_dummy.c)
+if(WIN32)
+ add_library(onnxifi_dummy onnx/onnxifi_dummy.c) # for ${arch}-windows-static triplets
JackBoosY marked this conversation as resolved.
Show resolved Hide resolved
+else()
+ add_library(onnxifi_dummy SHARED onnx/onnxifi_dummy.c)
+endif()

if(ONNXIFI_ENABLE_EXT)
add_definitions(-DONNXIFI_ENABLE_EXT=ON)
28 changes: 8 additions & 20 deletions ports/onnx/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# uwp: LOAD_LIBRARY_SEARCH_DEFAULT_DIRS undefined identifier
vcpkg_fail_port_install(ON_TARGET "uwp")
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
JackBoosY marked this conversation as resolved.
Show resolved Hide resolved

vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
Expand All @@ -9,15 +8,9 @@ vcpkg_from_github(
SHA512 a3eecc74ce4f22524603fb86367d21c87a143ba27eef93ef4bd2e2868c2cadeb724b84df58a429286e7824adebdeba7fa059095b7ab29df8dcea8777bd7f4101
PATCHES
fix-cmakelists.patch
wrap-onnxifi-targets.patch
)

if(VCPKG_TARGET_IS_WINDOWS)
string(COMPARE EQUAL "${VCPKG_CRT_LINKAGE}" "static" USE_STATIC_RUNTIME)
list(APPEND PLATFORM_OPTIONS
-DONNX_USE_MSVC_STATIC_RUNTIME=${USE_STATIC_RUNTIME}
)
endif()
string(COMPARE EQUAL "${VCPKG_CRT_LINKAGE}" "static" USE_STATIC_RUNTIME)

# ONNX_USE_PROTOBUF_SHARED_LIBS: find the library and check its file extension
find_library(PROTOBUF_LIBPATH NAMES protobuf PATHS ${CURRENT_INSTALLED_DIR}/bin ${CURRENT_INSTALLED_DIR}/lib REQUIRED)
Expand All @@ -30,35 +23,30 @@ endif()

vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
FEATURES
pybind11 BUILD_ONNX_PYTHON
pybind11 BUILD_ONNX_PYTHON
protobuf-lite ONNX_USE_LITE_PROTO
)

# Like protoc, python is required for codegen.
vcpkg_find_acquire_program(PYTHON3)

# PATH for .bat scripts can find 'python'
# PATH for .bat scripts so it can find 'python'
get_filename_component(PYTHON_DIR ${PYTHON3} PATH)
vcpkg_add_to_path(PREPEND ${PYTHON_DIR})

if("pybind11" IN_LIST FEATURES)
# When BUILD_ONNX_PYTHON, we need Development component. Give a hint for FindPython3
list(APPEND FEATURE_OPTIONS
-DPython3_ROOT_DIR=${CURRENT_INSTALLED_DIR}
)
endif()

vcpkg_cmake_configure(
SOURCE_PATH "${SOURCE_PATH}"
OPTIONS
${FEATURE_OPTIONS} ${PLATFORM_OPTIONS}
${FEATURE_OPTIONS}
-DPython3_EXECUTABLE=${PYTHON3}
-DONNX_ML=ON
-DONNX_GEN_PB_TYPE_STUBS=ON
-DONNX_USE_PROTOBUF_SHARED_LIBS=${USE_PROTOBUF_SHARED}
-DONNX_USE_LITE_PROTO=OFF
-DONNXIFI_ENABLE_EXT=OFF
-DONNX_USE_MSVC_STATIC_RUNTIME=${USE_STATIC_RUNTIME}
-DONNX_BUILD_TESTS=OFF
-DONNX_BUILD_BENCHMARKS=OFF
MAYBE_UNUSED_VARIABLES
ONNX_USE_MSVC_STATIC_RUNTIME
)

if("pybind11" IN_LIST FEATURES)
Expand Down
14 changes: 6 additions & 8 deletions ports/onnx/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,17 @@
{
"name": "onnx",
"version-semver": "1.9.0",
"port-version": 1,
"description": "Open standard for machine learning interoperability",
"homepage": "https://onnx.ai",
"license": "Apache-2.0",
"supports": "!uwp",
"dependencies": [
"protobuf",
{
"name": "protobuf",
"host": true
},
{
"name": "python3",
"host": true
},
{
"name": "vcpkg-cmake",
"host": true
Expand All @@ -24,11 +22,11 @@
}
],
"features": {
"protobuf-lite": {
Copy link
Member

Choose a reason for hiding this comment

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

Features can't "remove" functionality or control alternatives; this should probably be a feature named protobuf which does the full thing and is in the default-features set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. But I think default-features: ["protobuf"] can give misunderstanding.
Some users may think onnx[core,pybind11] is valid and it won't use protobuf.

protobuf in pybind11.dependencies will look weird.
But this one looks weird either in my opinion....

{
  "dependencies": [
    "protobuf",
    "python3"
  ],
  "default-features": [
    "protobuf"
  ],
  "features": {
    "protobuf": {
      "description": "Build with full protobuf library"
    },
    "protobuf-lite": {
      "description": "Use lite protobuf instead of full"
    },
    "pybind11": {
      "description": "Build Python binaries"
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Some users may think onnx[core,pybind11] is valid and it won't use protobuf.

I believe that is unavoidable.

protobuf in pybind11.dependencies will look weird.

You can add onnx[core,protobuf] as a dependency of the pybind11 feature to express dependencies between features.

Does using protobuf-lite remove the protobuf dependency? If so, the dependency should be moved to the protobuf feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add dependency to onnx[core,protobuf] for pybind11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, protobuf-lite also requires protobuf library and ptoroc in its tool.

"description": "Use lite protobuf instead of full"
},
"pybind11": {
"description": "Build Python binaries",
"dependencies": [
"pybind11"
]
"description": "Build Python binaries"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delete the dependency pybind11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary when I tried to use FimdPython3 with find_package pybind11. But doing that blocks search of NumPy component. Same as the PR note

Copy link
Member

Choose a reason for hiding this comment

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

That is, the port needs Python 3 pybind but our pybind11 port is for Python 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It was quite confusing to me. Let me explain with PR comments...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. commented below. #20112 (comment)

}
}
}
44 changes: 0 additions & 44 deletions ports/onnx/wrap-onnxifi-targets.patch
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4710,7 +4710,7 @@
},
"onnx": {
"baseline": "1.9.0",
"port-version": 0
"port-version": 1
},
"onnx-optimizer": {
"baseline": "0.2.6",
Expand Down
5 changes: 5 additions & 0 deletions versions/o-/onnx.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "bf1f2c3d7e83994b476592f29ea750152d71285f",
"version-semver": "1.9.0",
"port-version": 1
},
{
"git-tree": "b53c9c9e969928def925c57ea5ddcdfb09293693",
"version-semver": "1.9.0",
Expand Down