Skip to content

Synchronize patch files, fix resource compiler invocations in some situations#23855

Closed
zwassall-gentuity wants to merge 5 commits intomicrosoft:mainfrom
zwassall-gentuity:sync-patch-files
Closed

Synchronize patch files, fix resource compiler invocations in some situations#23855
zwassall-gentuity wants to merge 5 commits intomicrosoft:mainfrom
zwassall-gentuity:sync-patch-files

Conversation

@zwassall-gentuity
Copy link

Description

Deduplicates/synchronizes patch files in "cmake/patches" and "cmake/vcpkg-ports".

Motivation and Context

Addresses #23426 (comment) with Deduplicate/synchronize protobuf patch files:

This introduces some extra changes to the non-vcpkg configuration that were in cmake/vcpkg-ports/protobuf/protobuf_cmake.patch but not cmake/patches/protobuf/protobuf_cmake.patch. I believe that the changes are all welcome; in particular, the new changes to CMakeLists.txt in the patch file (from protocolbuffers/protobuf#10293) fix resource compiler invocations in some situations, as was the case for me using clang-cl on Windows.

The other deduplication/synchronization changes don't address immediate issues, but will ideally make future maintenance of these patch files less vulnerable to the possible mistake of updating one location but forgetting to consider the other.

This introduces some extra changes to the vcpkg configuration that were
in cmake/patches/onnx/onnx.patch but not
cmake/vcpkg-ports/onnx/binskim.patch, but they should be good.
This introduces some extra changes to the non-vcpkg configuration that
were in cmake/vcpkg-ports/protobuf/protobuf_cmake.patch but not
cmake/patches/protobuf/protobuf_cmake.patch. I believe that the changes
are all welcome; in particular, the new changes to CMakeLists.txt in the
patch file (from protocolbuffers/protobuf#10293)
fix resource compiler invocations in some situations, as was the case
for me using clang-cl on Windows.
Copy link
Author

Choose a reason for hiding this comment

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

This is the first of the 3 patch files for eigen3, all of which were unreferenced. I removed the files with the assumption that they are unnecessary, but if this is not correct, please let me know.

HEAD_REF master
PATCHES
protobuf_cmake.patch
PATCHES ../../patches/protobuf/protobuf_cmake.patch
Copy link
Author

Choose a reason for hiding this comment

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

I'm unsure how to test that a reference like this works as intended. Is there some CI step that will execute this and verify its validity?

@zwassall-gentuity
Copy link
Author

@microsoft-github-policy-service agree company="Gentuity"

@snnn
Copy link
Contributor

snnn commented Mar 10, 2025

/azp run Big Models, Linux Android Emulator QNN CI Pipeline, Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline

@snnn
Copy link
Contributor

snnn commented Mar 10, 2025

/azp run Linux QNN CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@snnn
Copy link
Contributor

snnn commented Mar 10, 2025

/azp run Windows CPU CI Pipeline, Windows GPU CUDA CI Pipeline, Windows GPU DML CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows x64 QNN CI Pipeline, Windows GPU WebGPU CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@snnn snnn closed this Jul 3, 2025
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.

2 participants