Conversation
e096f40 to
95bf787
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
95bf787 to
87f64ea
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 51180 in repo microsoft/vcpkg |
|
arm64_osx was canceled for some reasons. |
It wasn't actually cancelled; when a push was done in between and the build was cancelled and restarted, GitHub got the cancellation notice of the first build but something got lost delivering the 'started running again' for the second. arm64_osx actually completed now, but there are still legitimate build failures; for example: /vcpkg/scripts/azure-pipelines/../ci.feature.baseline.txt: error: onnxruntime[core,cuda]:x64-linux build failed but was expected to pass
note: if onnxruntime[cuda] succeeds when built with other features but not alone, consider adding `onnxruntime[core,cuda]:x64-linux=combination-fails`
note: if onnxruntime[cuda] always fails, consider adding `onnxruntime[cuda]:x64-linux=feature-fails`, which will mark this test as failing, and remove onnxruntime[cuda] from combined feature testing
note: if some features are required, consider effectively always enabling those parts in portfile.cmake for onnxruntime, or consider adding `onnxruntime[required-feature]=options` to include 'required-feature' in all tests
/vcpkg/scripts/azure-pipelines/../ci.feature.baseline.txt: error: onnxruntime[core,cuda,openvino,xnnpack]:x64-linux build failed but was expected to pass
note: consider adding `onnxruntime=fail`, `onnxruntime:x64-linux=fail`, `onnxruntime[core,cuda,openvino,xnnpack]:x64-linux=combination-fails`, or equivalent skips, or by marking mutually exclusive features as optionsMy guess is that we need (There are other failures, this is just the first one I ran into) |
|
The error on Linux seems to be: |
87f64ea to
75a98c5
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| - target_link_options(onnxruntime PRIVATE ${onnxruntime_DELAYLOAD_FLAGS}) | ||
| + target_link_options(onnxruntime INTERFACE ${onnxruntime_DELAYLOAD_FLAGS}) |
There was a problem hiding this comment.
Can you explain what this change is trying to do? (It seems strange to force customers to delayload but not delayload yourself.)
There was a problem hiding this comment.
Sorry, I don't remember why I had to do this, I'd have to undo it to see what error showed up.
|
|
||
| #include "core/common/flatbuffers.h" | ||
|
|
||
| +#if 0 |
There was a problem hiding this comment.
Should we be trying to regenerate these rather than removing Flatbuffers' version lock here?
There was a problem hiding this comment.
I don't know how to do that, and I don't know either where this idea that it needs to exactly match the version is coming from.
I believe it did break in the past but I'm not sure it is still the case with ongoing versions.
At least I didn't find anything in flatbuffer's documentation, and my deployment showed that it worked for me.
There was a problem hiding this comment.
@BillyONeal Please refer here to #49596 which still waits for VCPKG-Team-Review
| ) | ||
|
|
||
| if ("coreml" IN_LIST FEATURES) | ||
| vcpkg_from_github( |
There was a problem hiding this comment.
Should this be its own port? (I'm looking to see if that has been considered, not necessarily asking you to do it)
There was a problem hiding this comment.
onnxruntime takes a few files from that repo and compiles it itself, including generating some .proto file.
At least that's how it is currently done in onnxruntime.
Making another package? Maybe, but that'd make the build even more complex.
There was a problem hiding this comment.
The problem is that by doing it this way as a patch we are making vcpkg the thing that is whacking flatbuffers over the head, and customers quite reasonably can come to us saying "why did you patch this out, you introduced security bug XYZ" or similar.
I understand you might not be familiar with how to do this correctly but there is a reason many of these components were turned off in vcpkg's registry before.
If this merges then the vcpkg maintainers need to be able to explain why our product is doing it and what the implications of that are. For some kinds of build/packaging changes we're happy to speak confidently that we know more about build tools than most component owners. But explicitly disregarding something that an upstream maintainer has gone out of their way to do generally has a high bar. (See also https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#patching )
| # relocates the onnxruntime_providers_* binaries before vcpkg_copy_pdbs() | ||
| function(reolocate_ort_providers) | ||
| if(VCPKG_TARGET_IS_WINDOWS AND (VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")) | ||
| if(VCPKG_TARGET_IS_WINDOWS AND ((VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic") OR ("openvino" IN_LIST FEATURES))) |
There was a problem hiding this comment.
This seems like it can't be correct: turning on openvino changes how other providers are dealt with?
There was a problem hiding this comment.
openvino provider builds as shared library regardless of vcpkg's setting.
They force that in onnxruntime.
There was a problem hiding this comment.
We could otherwise say that openvino isn't supported in dynamic builds.
There was a problem hiding this comment.
openvino provider builds as shared library regardless of vcpkg's setting.
Doesn't that mean we should do something like if ("openvino" IN LIST FEATURES) vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) endif() ?
|
I'm maybe being a bit naive here, but onnx and onnxruntime are both Microsoft efforts. Could we imagine some internal collaboration between vcpkg and onnx* to make them a first class citizen? |
|
I'm going on holidays for a week, feel free to take this PR and modify/merge it 👍 |
From the perspective of vcpkg's curated registry, there is no reason to treat onnxruntime any differently than any other port. Just because maintainers of both happen to be employed at Microsoft does not mean that we magically know each others' userbases, design constraints, or inner workings. The rules vcpkg imposes on ports are about maintaining integrity of the registry itself because component owners rarely consider "what if two people did this" and "what about people who depend on something that depends on me" type of questions, for a large pile of historical reasons that boiled down to "just copy pasta their source is so much easier". |
For this reason I'm setting this to draft. When you come back and can speak to the particular patches feel free to re-mark "Ready for review" |
./vcpkg x-add-version --alland committing the result.