libonxx + 1.15.0 migration #105

hmaarrfk · 2023-11-05T12:15:29Z

Note that the windows library is still static

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-11-05T12:15:34Z

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:

The top level meta key oututs is unexpected
The recipe must have some tests.

conda-forge-webservices · 2023-11-05T12:34:24Z

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:

The outputs section contained an unexpected subsection name. tests is not a valid subsection name.

For recipe:

It looks like the 'onnx' output doesn't have any tests.

conda-forge-webservices · 2023-11-05T12:35:52Z

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.

…nda-forge-pinning 2023.11.02.10.11.59

recipe/0001-Link-to-abseil_dll.patch

hmaarrfk · 2023-11-06T03:26:00Z

I don't really understand what I am doing wrong on windows..... help appreciated.

traversaro · 2023-11-06T11:20:15Z

I don't really understand what I am doing wrong on windows..... help appreciated.

ONNX_API is used as export macro for both onnx_proto and onnx libraries. So, onnx_proto symbols get marked with __declspec(dllexport) also in onnx's compilation units, and so the linker complains that they are not found even if they are present in the linked import librar onnx_proto.lib .

A quick fix (I just tested locally, it seems to work) is to change the export macro for onnx-proto : traversaro/onnx@e7719fd . It is not ideal as non-CMake downstream users needs to manually set ONNX_PROTO_API , but better than not working. A better fix is to define ONNX_PROTO_API in an header, but unfortunately protobuf does not support including an arbitrary header inclusion in the generated headers, so downstream projects typically do that via external scripts, see protocolbuffers/protobuf#4198 for more details. If it is not a problem to expose all symbols of onnx_proto , an alternative simple solution is to set the WINDOWS_EXPORT_ALL_SYMBOLS CMake definition just for onnx_proto, and avoid to use any export pre-processor macro.

traversaro · 2023-11-06T11:21:10Z

recipe/0001-Link-to-abseil_dll.patch

+   endif()
+ endif()
+ add_onnx_global_defines(onnx_proto)
+@@ -528,7 +531,7 @@ target_include_directories(onnx PUBLIC


If you find protobuf to find_package(Protobuf CONFIG), this should be handled automatically by CMake.

Anyhow, if this works fine it is ok to use that.

traversaro · 2023-11-06T11:22:35Z

recipe/meta.yaml

+        - make  # [unix]
+      host:
+        - libprotobuf
+        - libabseil  # [win]


I guess this should be present also on non-Windows platforms, so the correctly run_exports are exported for abseil (as it is a public dependency of libprotobuf).

the linking analysis at the end doesn't make libaseil appear on unix

i think this is due to the fact that linux uses shared libraries

I see probably the protobuf headers that transitively include abseil are not used. Given that you are linking with https://github.com/conda-forge/onnx-feedstock/pull/105/files/16d193ff4ab9a19c6e551858659303148bb89e21#diff-f3725a55bf339595bf865fec73bda8ac99f283b0810c205442021f29c06eea9aR13 and not via find_package(Protobuf CONFIG), if those are used probably build will fail?

For reference, this was the discussion on libprotobuf now requiring linking to libabseil: conda-forge/conda-forge-pinning-feedstock#4075 (comment) .

recipe/bld_libonnx.bat

hmaarrfk · 2023-11-06T12:31:39Z

Thanks for the great explination.

Why is compilation not failing for #103 (comment)

recipe/bld_libonnx.bat

hmaarrfk · 2023-11-06T13:13:34Z

i see it is due to the fact that they aren't building shared libraries for windows.

hmaarrfk · 2023-11-06T14:02:49Z

I think WINDOWS_EXPORT_ALL_SYMBOLS is probably the best way to go.... but i'm not sure of any unintended effects.

hmaarrfk · 2023-11-06T23:30:13Z

maybe we can in CMake inject something into the header.

traversaro · 2023-11-07T08:42:30Z

maybe we can in CMake inject something into the header.

Just to understand, what is the goal now? Building as shared in Windows? In theory the patch in #105 (comment) should work fine, are you afraid the problem will create on non-cmake users of libonnx ?

hmaarrfk · 2023-11-07T12:21:34Z

are you afraid the problem will create on non-cmake users of libonnx

yes.

I would ideally also like to upstream whatever patch i make since these build patches are pretty hard to maintain.

hmaarrfk · 2023-11-07T13:45:50Z

It seems onnx worked around this issue by creating an other header file.

Maybe I have to move files around to create this shared library

https://github.com/onnx/onnx/blob/main/onnx/onnx_pb.h#L12

hmaarrfk force-pushed the libonxx branch from 9194c52 to 74c758d Compare November 5, 2023 12:36

hmaarrfk added 2 commits November 5, 2023 07:55

Try to build libonnx

d1987d7

MNT: Re-rendered with conda-build 3.27.0, conda-smithy 3.27.1, and co…

f93f62d

…nda-forge-pinning 2023.11.02.10.11.59

hmaarrfk force-pushed the libonxx branch from b7d0064 to f93f62d Compare November 5, 2023 12:55

hmaarrfk added 11 commits November 5, 2023 08:03

Try again

ff42035

Add libprotobuf

c6d5f9b

Try one more time

e43f272

Update

fd4856c

Up to 1.15.0

cada04b

Add back abseil patch

5873700

Remove debugging script

075fff6

remove zip

1253e5b

Fix windows build script

967327e

Update

4011658

fixup windows build

7927f98

hmaarrfk force-pushed the libonxx branch from 04f74b8 to 7927f98 Compare November 5, 2023 17:01

Update the CXX standard on windows

6914148

hmaarrfk changed the title ~~Libonxx~~ ibonxx + 1.15.0 migration Nov 5, 2023

hmaarrfk changed the title ~~ibonxx + 1.15.0 migration~~ libonxx + 1.15.0 migration Nov 5, 2023

Update patch for windows

f95cd1b

hmaarrfk commented Nov 5, 2023

View reviewed changes

recipe/0001-Link-to-abseil_dll.patch Outdated Show resolved Hide resolved

Apply suggestions from code review

bda728f

hmaarrfk commented Nov 5, 2023

View reviewed changes

recipe/0001-Link-to-abseil_dll.patch Outdated Show resolved Hide resolved

hmaarrfk added 2 commits November 5, 2023 15:27

Update recipe/0001-Link-to-abseil_dll.patch

66650c1

Update patch one last time

956bad9

hmaarrfk mentioned this pull request Nov 5, 2023

[WIP] Add pytorch dependencies conda-forge/staged-recipes#19103

Open

9 tasks

One last time

1ea3744

hmaarrfk added 2 commits November 5, 2023 22:14

Try adding more flags

534376d

Small updates

16d193f

hmaarrfk marked this pull request as ready for review November 6, 2023 03:26

hmaarrfk requested review from cbourjau, ezyang, janjagusch, marcelotrevisani and xhochy as code owners November 6, 2023 03:26

traversaro reviewed Nov 6, 2023

View reviewed changes

This was referenced Nov 6, 2023

Can't build from source via cmake onnx/onnx#5279

Open

[onnx] Allow dynamic builds microsoft/vcpkg#34899

Closed

hmaarrfk added 2 commits November 6, 2023 07:24

more defines

9694f50

Update build_libonnx.sh

cc053ed

hmaarrfk commented Nov 6, 2023

View reviewed changes

recipe/bld_libonnx.bat Show resolved Hide resolved

Update recipe/bld_libonnx.bat

391d1ad

hmaarrfk commented Nov 6, 2023

View reviewed changes

recipe/bld_libonnx.bat Outdated Show resolved Hide resolved

no shared libs on windows

f2a4c8a

hmaarrfk added 2 commits November 6, 2023 08:19

Update meta.yaml

6203703

Update bld_onnx.bat

3c75ce1

hmaarrfk mentioned this pull request Mar 9, 2024

Build this as libonnx #77

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libonxx + 1.15.0 migration #105

libonxx + 1.15.0 migration #105

hmaarrfk commented Nov 5, 2023 •

edited

Loading

conda-forge-webservices bot commented Nov 5, 2023

conda-forge-webservices bot commented Nov 5, 2023

conda-forge-webservices bot commented Nov 5, 2023

hmaarrfk commented Nov 6, 2023

traversaro commented Nov 6, 2023

traversaro Nov 6, 2023

traversaro Nov 6, 2023

traversaro Nov 6, 2023

hmaarrfk Nov 6, 2023

hmaarrfk Nov 6, 2023

traversaro Nov 7, 2023

traversaro Nov 7, 2023

hmaarrfk commented Nov 6, 2023

hmaarrfk commented Nov 6, 2023

hmaarrfk commented Nov 6, 2023

hmaarrfk commented Nov 6, 2023

traversaro commented Nov 7, 2023

hmaarrfk commented Nov 7, 2023

hmaarrfk commented Nov 7, 2023 •

edited

Loading

libonxx + 1.15.0 migration #105

Are you sure you want to change the base?

libonxx + 1.15.0 migration #105

Conversation

hmaarrfk commented Nov 5, 2023 • edited Loading

conda-forge-webservices bot commented Nov 5, 2023

conda-forge-webservices bot commented Nov 5, 2023

conda-forge-webservices bot commented Nov 5, 2023

hmaarrfk commented Nov 6, 2023

traversaro commented Nov 6, 2023

traversaro Nov 6, 2023

Choose a reason for hiding this comment

traversaro Nov 6, 2023

Choose a reason for hiding this comment

traversaro Nov 6, 2023

Choose a reason for hiding this comment

hmaarrfk Nov 6, 2023

Choose a reason for hiding this comment

hmaarrfk Nov 6, 2023

Choose a reason for hiding this comment

traversaro Nov 7, 2023

Choose a reason for hiding this comment

traversaro Nov 7, 2023

Choose a reason for hiding this comment

hmaarrfk commented Nov 6, 2023

hmaarrfk commented Nov 6, 2023

hmaarrfk commented Nov 6, 2023

hmaarrfk commented Nov 6, 2023

traversaro commented Nov 7, 2023

hmaarrfk commented Nov 7, 2023

hmaarrfk commented Nov 7, 2023 • edited Loading

hmaarrfk commented Nov 5, 2023 •

edited

Loading

hmaarrfk commented Nov 7, 2023 •

edited

Loading