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

Rework some external targets to ease building with -DFETCHCONTENT_FULLY_DISCONNECTED=ON #15323

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Apr 1, 2023

Description

Rework some external targets to ease building with -DFETCHCONTENT_FULLY_DISCONNECTED=ON
This will allow package managers to more easily provide an onnxruntime package by reducing the amount of patching needed downstream at each version.

Motivation and Context

Availability of onnxruntime in some C++ package managers
#7150
conan-io/conan-center-index#16699
microsoft/vcpkg#20548

My initial intent is to get this in conan but the PR would most likely be useful (though not tested) to vcpkg as well (and maybe others).
I tried to get only a first batch of not too specific patches (i.e. not specific to conan).

The first commit reworks flatbuffers and just extends what @snnn did in #13991
The second commit reworks pytorch_cpuinfo
The third commit reworks google_nsync

mayeut added 3 commits April 1, 2023 14:49
`flatbuffers::flatbuffers` shall be used to allow building with `-DFETCHCONTENT_FULLY_DISCONNECTED=ON`
`cpuinfo::cpuinfo` shall be used to allow building with `-DFETCHCONTENT_FULLY_DISCONNECTED=ON`
`nsync::nsync_cpp` shall be used to allow building with `-DFETCHCONTENT_FULLY_DISCONNECTED=ON`
onnxruntime_add_include_to_target(onnxruntime_common cpuinfo)
list(APPEND onnxruntime_EXTERNAL_LIBRARIES cpuinfo clog)
onnxruntime_add_include_to_target(onnxruntime_common cpuinfo::cpuinfo)
list(APPEND onnxruntime_EXTERNAL_LIBRARIES cpuinfo::cpuinfo)
Copy link
Member

Choose a reason for hiding this comment

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

Why clog is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clog is never referenced by onnxruntime directly.
It's only a transitive dependency correctly defined & handled by the cpuinfo::cpuinfo target and thus can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, after this change the iOS packaging pipeline started failing. Adding the explicit clog dependency back in #15363.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the head's up.
I'll check the specific workflow https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=945506&view=logs&j=f7cc61a9-cc70-56e7-b06c-4668ca17e426&t=f59b4803-6cfe-5208-3daa-194b51f8b8b4 once I get a bit of time, there's still something I'd like to understand and maybe propose a follow up on #15363 once I do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edgchen1, @snnn,

just FYI, when building locally 2e52de2 with #15363 reverted, I don't observe the linking problem (but I don't have the same environment as the builder on Azure, notably CMake/Xcode versions).

./build.sh --build_shared --use_coreml --use_xnnpack --ios --ios_sysroot iphonesimulator --osx_arch x86_64 --apple_deploy_target 11.0 --use_xcode --config RelWithDebInfo --build_apple_framework --parallel --path_to_protoc_exe /Users/Matt/Dev/onnxruntime/build/MacOS/RelWithDebInfo/_deps/protobuf-build/RelWithDebInfo/protoc leads to some other error:

In file included from /Users/Matt/Dev/onnxruntime/onnxruntime/core/session/inference_session_utils.cc:6:
In file included from /Users/Matt/Dev/onnxruntime/onnxruntime/core/session/inference_session_utils.h:13:
In file included from /Users/Matt/Dev/onnxruntime/onnxruntime/core/session/inference_session.h:18:
/Users/Matt/Dev/onnxruntime/onnxruntime/core/framework/kernel_registry_manager.h:84:17: error: 'visit<(lambda at
      /Users/Matt/Dev/onnxruntime/onnxruntime/core/framework/kernel_registry_manager.h:84:23), const std::variant<onnxruntime::OpSchemaKernelTypeStrResolver, onnxruntime::KernelTypeStrResolver> &,
      void>' is unavailable: introduced in iOS 12.0
    return std::visit([](auto&& r) -> const IKernelTypeStrResolver& { return r; }, kernel_type_str_resolver_variant_);
                ^
In file included from /Users/Matt/Dev/onnxruntime/onnxruntime/core/session/inference_session_utils.cc:6:
In file included from /Users/Matt/Dev/onnxruntime/onnxruntime/core/session/inference_session_utils.h:6:
In file included from /Users/Matt/Dev/onnxruntime/onnxruntime/core/flatbuffers/schema/ort.fbs.h:7:
In file included from /Users/Matt/Dev/onnxruntime/build/iOS/RelWithDebInfo/_deps/flatbuffers-src/include/flatbuffers/flatbuffers.h:20:
In file included from /Users/Matt/Dev/onnxruntime/build/iOS/RelWithDebInfo/_deps/flatbuffers-src/include/flatbuffers/base.h:41:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator16.4.sdk/usr/include/c++/v1/string:549:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator16.4.sdk/usr/include/c++/v1/memory:877:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator16.4.sdk/usr/include/c++/v1/iterator:684:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator16.4.sdk/usr/include/c++/v1/__iterator/common_iterator.h:22:
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator16.4.sdk/usr/include/c++/v1/variant:1710:20: note: 'visit<(lambda at
      /Users/Matt/Dev/onnxruntime/onnxruntime/core/framework/kernel_registry_manager.h:84:23), const std::variant<onnxruntime::OpSchemaKernelTypeStrResolver, onnxruntime::KernelTypeStrResolver> &,
      void>' has been explicitly marked unavailable here
    decltype(auto) visit(_Visitor&& __visitor, _Vs&&... __vs) {

I had to bump the minimum version to 12.0 (& change the simulator to iPhone 3rd gen because I don't have 2nd gen with my Xcode version) to get the build passing.

I won't propose a follow up on #15363 since I can't reproduce the issue and can live happily with the change in it even though I'm a bit puzzled to why it's needed in the first place.

@snnn
Copy link
Member

snnn commented Apr 3, 2023

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, Linux QNN CI Pipeline, MacOS CI Pipeline

@snnn
Copy link
Member

snnn commented Apr 3, 2023

/azp run ONNX Runtime Web CI Pipeline, Windows ARM64 QNN CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed

@azure-pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@snnn snnn merged commit 85bb133 into microsoft:main Apr 4, 2023
mszhanyi added a commit that referenced this pull request Apr 4, 2023
…NTENT_FULLY_DISCONNECTED=ON` (#15323)"

This reverts commit 85bb133.
adityagoel4512 pushed a commit to adityagoel4512/onnxruntime that referenced this pull request Apr 5, 2023
…LLY_DISCONNECTED=ON` (microsoft#15323)

### Description
Rework some external targets to ease building with
`-DFETCHCONTENT_FULLY_DISCONNECTED=ON`
This will allow package managers to more easily provide an onnxruntime
package by reducing the amount of patching needed downstream at each
version.

### Motivation and Context
Availability of onnxruntime in some C++ package managers
microsoft#7150
conan-io/conan-center-index#16699
microsoft/vcpkg#20548

My initial intent is to get this in conan but the PR would most likely
be useful (though not tested) to vcpkg as well (and maybe others).
I tried to get only a first batch of not too specific patches (i.e. not
specific to conan).

The first commit reworks `flatbuffers` and just extends what @snnn did
in microsoft#13991
The second commit reworks `pytorch_cpuinfo`
The third commit reworks `google_nsync`
@mayeut mayeut deleted the fix-targets-no-fetch branch January 20, 2025 20:24
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.

3 participants