Skip to content

Conversation

@smanolloff
Copy link
Contributor

@smanolloff smanolloff commented Nov 23, 2025

This PR contains changes needed to migrate MMAI from libtorch to onnxruntime as @Laserlicht suggested here

It adds onnxruntime/1.18.1 requirement with several patches address android-arm64 and windows-x86 compilation errors, as well as conan recipe errors from transitive dependencies.

Please take a look and let me know if you have any comments.

The changes were tested with VCMI (develop branch) on MacOS 15 (arm), Windows 10 (x64), iOS 18 and Android 13 without noticing any issue.

This reverts commit 56f3512.

Removing CreateFile2 calls was insufficient to enable win7 support.
There are win8+ API calls which are not straightforward to replace:

    onnxruntime\core\platform\windows\env_time.cc(31): error C2065: 'GetSystemTimePreciseAsFileTime': undeclared identifier
@smanolloff
Copy link
Contributor Author

smanolloff commented Nov 25, 2025

Added conan patch which allows to keep macos-intel 10.13 and ios 12.0 min OS versions.
Unfortunately, my attempt at compiling onnxruntime on win7 was not successful: there are other calls such as GetSystemTimePreciseAsFileTime which are not available in the windows 7 API and are seem more difficult to replace. So the win7 / win8 build split remains.

@GeorgeK1ng
Copy link

GeorgeK1ng commented Nov 25, 2025

I think there is issue with FILE_READ_ATTRIBUTES vs GENERIC_READ in W7 patch?

Also keeping 192 which means v142 toolset is needed to keep proper support for builded components for Windows 7.

@smanolloff
Copy link
Contributor Author

I think there is issue with FILE_READ_ATTRIBUTES vs GENERIC_READ in W7 patch?

A hasty copy-paste on my side it seems, but that's irrelevant as I reverted the commit due to other compile errors.

Also keeping 192 which means v142 toolset is needed to keep proper support for builded components for Windows 7.

Thanks for pointing that out, I was under the impression "if it compiles, it runs" :) The compiler version is back to 192 for windows 7 now.

@GeorgeK1ng
Copy link

GeorgeK1ng commented Nov 26, 2025

Seems your patches were almost close. This should be Windows 7 compatible patch https://github.com/GeorgeK1ng/vcmi-dependencies/actions/runs/19713613028 - compiled just fine. So you need to grab there Windows 7 patch and recipe ones to allow v142 toolset.

@smanolloff smanolloff force-pushed the mmai branch 2 times, most recently from 0e42897 to 76afc17 Compare November 27, 2025 09:33
@smanolloff
Copy link
Contributor Author

smanolloff commented Nov 27, 2025

@GeorgeK1ng Thank you! I have added your suggested patch, finally the win7/win8 split is removed - the last remaining conan change in the PR has been dealt with.

Copy link
Collaborator

@kambala-decapitator kambala-decapitator left a comment

Choose a reason for hiding this comment

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

please also update PR description to match the current state

@IvanSavenko
Copy link
Member

@kambala-decapitator any further comments?

@kambala-decapitator
Copy link
Collaborator

will check over the weekend

Copy link
Contributor Author

@smanolloff smanolloff left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback. I will apply the suggested changes.

@smanolloff
Copy link
Contributor Author

@kambala-decapitator I have made the requested changes. Please let me know if you have other suggestions.

@smanolloff
Copy link
Contributor Author

@kambala-decapitator I have replaced jq with a python snippet, can you take a look

@IvanSavenko IvanSavenko merged commit 1857724 into vcmi:main Dec 10, 2025
9 checks passed
@kambala-decapitator
Copy link
Collaborator

wow, windows and android dependencies have seriously increased in size

@Laserlicht
Copy link
Contributor

wow, windows and android dependencies have seriously increased in size

ML libraries are huge in general (seems that size has not much priority). I think libonnx is still relative small compared e.g. to others.

@kambala-decapitator
Copy link
Collaborator

yes, the size isn't a big deal, just an observation

kambala-decapitator added a commit that referenced this pull request Dec 10, 2025
kambala-decapitator added a commit that referenced this pull request Dec 10, 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.

5 participants