Add Windows support #56

traversaro · 2023-04-14T13:18:29Z

Based on #10 .

Main important things to know:

Due to the fact that a onnxruntime.dll is shipped by Windows in C:\Windows\system32\, we can't use that name in our package. To avoid this problem, we rename our dll to onnxruntime_conda.dll, see Add Windows support #56 (comment) .
Even if we did not explicitly dependenced on protobuf, a conda-forge version of protobuf was used, even if we agreed to concentrated on using vendored dependencies for now (). So we pass CMAKE_DISABLE_FIND_PACKAGE_Protobuf=ON to make sure that the vendored Protobuf is used, see Add Windows support #56 (comment) .

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

conda-forge-webservices · 2023-04-14T13:18:36Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

traversaro · 2023-04-14T13:18:48Z

@conda-forge-admin, please rerender

cbourjau

I don't have much experience packaging for Windows but maybe we can try to get things to build without the tests first?

recipe/bld.bat

traversaro · 2023-04-21T16:13:48Z

I don't have much experience packaging for Windows but maybe we can try to get things to build without the tests first?

Good point, I kind of left this in an incomplete state, as I think before going for Windows probably it could make sense to go for de-venderoing, that should simplify also the Windows build: #57 .

traversaro · 2023-04-21T16:15:36Z

I don't have much experience packaging for Windows but maybe we can try to get things to build without the tests first?

Good point, I kind of left this in an incomplete state, as I think before going for Windows probably it could make sense to go for de-venderoing, that should simplify also the Windows build: #57 .

Ahs sorry, I did not see your comment there.

cbourjau · 2023-05-03T13:05:03Z

@traversaro are there any updates or do you have open questions? I think supporting Windows would be very nice!

traversaro · 2023-06-09T14:31:45Z

@traversaro are there any updates or do you have open questions? I think supporting Windows would be very nice!

Sorry for the radio silence, see #57 (comment) for more context. I plan to work on this in the near future.

traversaro · 2023-06-09T14:35:11Z

@conda-forge-admin, please rerender

traversaro · 2023-06-11T07:51:36Z

Cool, Windows fails with:

2023-06-09T16:46:05.6721585Z %SRC_DIR%\onnxruntime\test\contrib_ops\longformer_attention_op_test.cc : fatal error C1085: Cannot write compiler generated file: '%SRC_DIR%\build-ci\Release\CMakeFiles\onnxruntime_test_all.dir\D_\bld\onnxruntime_1686326253701\work\onnxruntime\test\contrib_ops\longformer_attention_op_test.cc.obj': No space left on device

While I could disable tests to save some space, let me check if there is some other way.

traversaro · 2023-06-11T07:58:59Z

Cool, Windows fails with:

2023-06-09T16:46:05.6721585Z %SRC_DIR%\onnxruntime\test\contrib_ops\longformer_attention_op_test.cc : fatal error C1085: Cannot write compiler generated file: '%SRC_DIR%\build-ci\Release\CMakeFiles\onnxruntime_test_all.dir\D_\bld\onnxruntime_1686326253701\work\onnxruntime\test\contrib_ops\longformer_attention_op_test.cc.obj': No space left on device

While I could disable tests to save some space, let me check if there is some other way.

I tried with b4a85b3 .

traversaro · 2023-06-11T07:59:06Z

@conda-forge-admin, please rerender

conda-forge-webservices · 2023-06-11T08:40:33Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

traversaro · 2023-06-11T08:52:52Z

While debugging locally, I found quite a tricky problem. By default the onnxruntime build a onnxruntime.dll file. If I try to compile the test against it, I get the error:

(onnxtest) C:\src\onnxruntime-feedstock\recipe>.\test.exe
The given version [15] is not supported, only version 1 to 7 is supported in this build.

Why this happens? Basically because apparently Windows ships a onnxruntime.dll file in C:\Windows\system32\onnxruntime.dll, and according to .dll search order in Windows ( https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order ) anything that is found in C:\Windows\system32 has a higher priority w.r.t. to anything found in the PATH . See microsoft/onnxruntime#11230 for more details.

To avoid this issue, probably we should rename onnxruntime.dll to onnxruntime_conda.dll, as done in the past for other packages such as qt-main. The main problem of this approach is that at the moment onnxruntime does not ship any .pc pkg-config file or onnxruntime-config.cmake CMake config files, so basically this name difference needs to be accounted for in downstream build systems.

traversaro · 2023-06-11T09:28:49Z

@conda-forge-admin, please rerender

github-actions · 2023-06-11T09:30:29Z

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/onnxruntime-feedstock/actions/runs/5234695625.

conda-forge-webservices · 2023-06-11T09:31:16Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

traversaro · 2023-06-11T09:32:30Z

@conda-forge-admin, please rerender

github-actions · 2023-06-11T09:34:43Z

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/onnxruntime-feedstock/actions/runs/5234710097.

traversaro · 2023-06-11T12:57:58Z

We are almost there:

2023-06-11T10:39:23.5680489Z [1133/1142] Linking CXX executable onnxruntime_test_all.exe
2023-06-11T10:39:23.5680829Z FAILED: onnxruntime_test_all.exe 
2023-06-11T10:39:23.5694971Z cmd.exe /C "cmd.exe /C "cd /D %SRC_DIR%\build-ci\Release && %BUILD_PREFIX%\Library\bin\cmake.exe -E copy_directory C:/bld/onnxruntime-novec_1686477126258/work/onnxruntime/test/testdata C:/bld/onnxruntime-novec_1686477126258/work/build-ci/Release/testdata && cd /D %SRC_DIR%\build-ci\Release && %BUILD_PREFIX%\Library\bin\cmake.exe -E copy_directory C:/bld/onnxruntime-novec_1686477126258/work/samples C:/bld/onnxruntime-novec_1686477126258/work/build-ci/Release/samples && cd %SRC_DIR%\build-ci\Release" && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\onnxruntime_test_all.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\HostX64\x64\link.exe /nologo @CMakeFiles\onnxruntime_test_all.rsp  /out:onnxruntime_test_all.exe /implib:onnxruntime_test_all.lib /pdb:onnxruntime_test_all.pdb /version:0.0 /machine:x64 /DYNAMICBASE /guard:cf /INCREMENTAL:NO /OPT:REF,ICF,LBR /INCREMENTAL:NO /LTCG /subsystem:console  /CETCOMPAT /INCREMENTAL:NO /LTCG  && cd ."
2023-06-11T10:39:23.7336562Z LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\HostX64\x64\link.exe /nologo @CMakeFiles\onnxruntime_test_all.rsp /out:onnxruntime_test_all.exe /implib:onnxruntime_test_all.lib /pdb:onnxruntime_test_all.pdb /version:0.0 /machine:x64 /DYNAMICBASE /guard:cf /INCREMENTAL:NO /OPT:REF,ICF,LBR /INCREMENTAL:NO /LTCG /subsystem:console /CETCOMPAT /INCREMENTAL:NO /LTCG /MANIFEST /MANIFESTFILE:onnxruntime_test_all.exe.manifest" failed (exit code 1257) with the following output:
2023-06-11T10:39:23.7338635Z LINK : warning LNK4044: unrecognized option '/lpthreads'; ignored
2023-06-11T10:39:23.7339557Z 
2023-06-11T10:39:23.7339557Z 
2023-06-11T10:39:23.7340056Z fatal error C1002: compiler is out of heap space in pass 2
2023-06-11T10:39:23.7340408Z 
2023-06-11T10:39:23.7341366Z LINK : fatal error LNK1257: code generation failed
2023-06-11T10:39:23.7341752Z

The compiler is out of heap space, so probably we need to reduce the number of concurrent threads.

traversaro · 2023-06-11T14:39:37Z

To avoid this issue, probably we should rename onnxruntime.dll to onnxruntime_conda.dll, as done in the past for other packages such as qt-main. The main problem of this approach is that at the moment onnxruntime does not ship any .pc pkg-config file or onnxruntime-config.cmake CMake config files, so basically this name difference needs to be accounted for in downstream build systems.

I implement this solution with patch 4a08914 . Linking with onnxruntime on Windows requires using (or searching for in the CMake case, see for example ami-iit/bipedal-locomotion-framework#686) the name onnxruntime_conda. This problem will be mitigated once onnxruntime provides .pc pkg-config files or CMake's onnxruntime-config.cmake file, see for example microsoft/onnxruntime#8919, as in that case downstream projects would simply link onnxruntime::onnxruntime.

traversaro · 2023-06-11T14:40:13Z

Once/if the CI is happy/green, the PR is ready for review.

traversaro · 2023-06-12T06:48:25Z

Even with parallel 1, the win win_64_python3.10.____cpythonsuffix build takes the same time and still fails with compiler is out of heap space in pass 2, so I suspect the parallel option is not correctly transmitted. I just tried locally and that seems the case.

traversaro · 2023-06-14T02:13:47Z

Related issues:

traversaro · 2023-06-14T02:24:03Z

As actually also Linux uses gtest/gmock from conda-forge, to be less invasive but just force the use of the vendored protobuf I pass the CMAKE_DISABLE_FIND_PACKAGE_Protobuf=ON to just avoid the finding and using the conda-forge's protobuf.

traversaro · 2023-06-14T14:27:03Z

@conda-forge-admin, please rerender

traversaro · 2023-06-14T15:59:48Z

@conda-forge-admin, please rerender

traversaro · 2023-06-15T15:51:30Z

For some reason the import onnxruntime test is failing with error:

2023-06-14T17:58:49.2006709Z import: 'onnxruntime'
2023-06-14T17:58:49.5311539Z import: 'onnxruntime'
2023-06-14T17:58:49.5696475Z The system cannot find the path specified.
2023-06-14T17:58:49.5727221Z The filename, directory name, or volume label syntax is incorrect.
2023-06-14T17:58:50.2280836Z Tests failed for onnxruntime-1.15.0-py310h0b3afb7_1.conda - moving package to D:\bld\broken

However, I tested locally the package onnxruntime-1.15.0-py310h0b3afb7_1.conda and import onnxruntime is working fine.

traversaro · 2023-06-15T16:12:42Z

Related errors:

traversaro · 2023-06-19T11:09:32Z

For some reason the import onnxruntime test is failing with error:
2023-06-14T17:58:49.2006709Z import: 'onnxruntime'
2023-06-14T17:58:49.5311539Z import: 'onnxruntime'
2023-06-14T17:58:49.5696475Z The system cannot find the path specified.
2023-06-14T17:58:49.5727221Z The filename, directory name, or volume label syntax is incorrect.
2023-06-14T17:58:50.2280836Z Tests failed for onnxruntime-1.15.0-py310h0b3afb7_1.conda - moving package to D:\bld\broken
However, I tested locally the package onnxruntime-1.15.0-py310h0b3afb7_1.conda and import onnxruntime is working fine.

I tested this locally, and I think I was barking at the wrong tree, the problem were in the run_test.bat, I fixed some errors in ce3edc3, let's see if this fix the errors.

conda-forge-webservices · 2023-06-19T14:10:21Z

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-webservices · 2023-06-19T15:12:24Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

traversaro · 2023-06-19T15:12:39Z

@conda-forge-admin, please rerender

github-actions · 2023-06-19T15:14:26Z

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/onnxruntime-feedstock/actions/runs/5313547709.

cbourjau

Thanks so much for all your effort! Congratulations on finally getting to that ✅ -state! As I said earlier, I don't have experience in packaging for Windows. Maybe @xhochy or @janjagusch are in a better position in this regard?

recipe/meta.yaml

recipe/16337.patch

traversaro · 2023-06-19T15:57:57Z

Congratulations on finally getting to that ✅ -state!

I am afraid that the CI checks are actually not running due to a re-rendering problem.

traversaro · 2023-06-19T19:19:56Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2023.06.19.09.23.46

traversaro · 2023-06-20T06:53:33Z

@conda-forge/onnxruntime the PR is now ready for review, I summarized the main things to know in the original PR message.

recipe/meta.yaml

Co-authored-by: Jan Tilly <jantilly@gmail.com>

jtilly · 2023-06-21T09:45:00Z

Thank you, @traversaro!

traversaro · 2023-06-21T09:54:55Z

Thanks to everyone that provided feedback and for merging the PR!

traversaro requested review from cbourjau, janjagusch, jtilly and xhochy as code owners April 14, 2023 13:18

This was referenced Apr 14, 2023

Add support for onnxruntime just on Linux with apt robotology/robotology-superbuild#1387

Merged

De-vendor vendored dependencies #57

Open

cbourjau reviewed Apr 21, 2023

View reviewed changes

recipe/bld.bat Outdated Show resolved Hide resolved

traversaro force-pushed the addwin branch from 243de59 to a09a9a9 Compare June 9, 2023 14:34

traversaro mentioned this pull request Jun 11, 2023

MANN: support Windows ami-iit/bipedal-locomotion-framework#686

Merged

Add Windows support

ecc21f6

traversaro force-pushed the addwin branch from 7e8e451 to ecc21f6 Compare June 19, 2023 15:12

cbourjau reviewed Jun 19, 2023

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

recipe/16337.patch Show resolved Hide resolved

Update azure-pipelines-osx.yml

6dde8ec

conda-forge-webservices[bot] and others added 2 commits June 19, 2023 19:21

MNT: Re-rendered with conda-build 3.25.0, conda-smithy 3.23.1, and co…

e4b9d7a

…nda-forge-pinning 2023.06.19.09.23.46

Update meta.yaml

7374514

jtilly reviewed Jun 20, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

Update recipe/meta.yaml

037c5d3

Co-authored-by: Jan Tilly <jantilly@gmail.com>

xhochy approved these changes Jun 21, 2023

View reviewed changes

jtilly merged commit 8365d36 into conda-forge:main Jun 21, 2023

traversaro mentioned this pull request Jun 22, 2023

Add running the testsuite step on non-Windows platforms #62

Merged

5 tasks

This was referenced Jul 31, 2023

Try building on Windows #10

Closed

Windows #31

Closed

traversaro mentioned this pull request Jan 25, 2024

Add dependency on onnruntime-cpp also on conda if ROBOTOLOGY_ENABLE_DYNAMICS_FULL_DEPS is enabled robotology/robotology-superbuild#1575

Merged

Add Windows support #56

Add Windows support #56

Conversation

traversaro commented Apr 14, 2023 • edited Loading

conda-forge-webservices bot commented Apr 14, 2023

traversaro commented Apr 14, 2023

cbourjau left a comment

Choose a reason for hiding this comment

traversaro commented Apr 21, 2023

traversaro commented Apr 21, 2023

cbourjau commented May 3, 2023

traversaro commented Jun 9, 2023

traversaro commented Jun 9, 2023

traversaro commented Jun 11, 2023

traversaro commented Jun 11, 2023

traversaro commented Jun 11, 2023

conda-forge-webservices bot commented Jun 11, 2023

traversaro commented Jun 11, 2023

traversaro commented Jun 11, 2023

github-actions bot commented Jun 11, 2023

conda-forge-webservices bot commented Jun 11, 2023

traversaro commented Jun 11, 2023

github-actions bot commented Jun 11, 2023

traversaro commented Jun 11, 2023

traversaro commented Jun 11, 2023

traversaro commented Jun 11, 2023

traversaro commented Jun 12, 2023 • edited Loading

traversaro commented Jun 14, 2023

traversaro commented Jun 14, 2023

traversaro commented Jun 14, 2023

traversaro commented Jun 14, 2023

traversaro commented Jun 15, 2023

traversaro commented Jun 15, 2023

traversaro commented Jun 19, 2023

conda-forge-webservices bot commented Jun 19, 2023

conda-forge-webservices bot commented Jun 19, 2023

traversaro commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

cbourjau left a comment

Choose a reason for hiding this comment

traversaro commented Jun 19, 2023

traversaro commented Jun 19, 2023

traversaro commented Jun 20, 2023 • edited Loading

jtilly commented Jun 21, 2023

traversaro commented Jun 21, 2023

traversaro commented Apr 14, 2023 •

edited

Loading

traversaro commented Jun 12, 2023 •

edited

Loading

traversaro commented Jun 20, 2023 •

edited

Loading