search

Torsion-Audio / Scyclone

Real-time Neural Timbre Transfer
Other
395 stars 12 forks source link

Update macOS onnx to v1.17 Universal build #10

Open kcoul opened 8 months ago

kcoul commented 8 months ago

Since the Universal build is over 100MB, this PR commits a .zip file of it instead, which CMake unzips when a macOS build target is detected.

This should fix https://github.com/Torsion-Audio/Scyclone/issues/5

kcoul commented 8 months ago

This should remain as a draft until Windows is also updated to v.1.17 of ONNX, otherwise there would be a header/lib mismatch.

faressc commented 7 months ago

Both onnxruntime libs in Scyclone are universal libs already. The problem in #5 seems to be a different one. Nonetheless at some point we should update our onnxruntime version...

kcoul commented 7 months ago

Ah I see, yes you are right, I just verified that myself. In that case it should be possible to just pick one of them and link to it for either flavour of macOS build. I am going to try working with one of them in my fork and see how I get along with it compared to the one I built myself (I think I used ort-builder as well). I will report back. I'll leave this draft PR open awhile longer in case I discover anything new.

faressc commented 7 months ago

Sounds good. Are you facing the same issues with the arm version like #5?

kcoul commented 7 months ago

@faressc no it seems fine so far! You might find my fork interesting as I have been trying to get Scyclone working on various platforms, with good success:

https://github.com/Torsion-Audio/Scyclone/compare/main...kcoul:Scyclone:main

faressc commented 7 months ago

This looks interesting. Linux support is something we are considering implementing in the near future. So thanks for your input on this! But we are trying to build a static lib with the ort builder and in your fork you are linking against a dynamic onnxruntime library. We use the static libs because we try to keep Scyclone free of runtime dependencies (other than the classic JUCE dependencies). The WASM support is also exciting. So you got Scyclone working in a browser?

kcoul commented 7 months ago

This looks interesting. Linux support is something we are considering implementing in the near future. So thanks for your input on this! But we are trying to build a static lib with the ort builder and in your fork you are linking against a dynamic onnxruntime library. We use the static libs because we try to keep Scyclone free of dependencies (other than the classic JUCE dependencies). The WASM support is also exciting. So you got Scyclone working in a browser?

With the Linux static lib of onnxruntime issue, I ran into some missing linker symbols in the Linux static library I produced from ort-builder, that was tricky to figure out. There are some commits related to that in my fork. An .mri file must be created listing the static libs to be merge together, then run ar -M < path_to_mri_file - ort-builder repo does not currently have this yet.

https://github.com/Torsion-Audio/Scyclone/commit/013f5c80b6e2906798b673a98f86cdfdcd15adbc https://github.com/olilarkin/ort-builder/blob/main/build-linux.sh

A combined static lib for Linux that merged the same libs that ort-builder does for macOS and Windows still produced the linker errors, so I gave up and switched to dynamic at that point.

With WASM, it was very close but I ran into an issue with the approach I was using (Projucer-generated Makefile for Linux modified for Emscripten). I would like to try again using CMake+Emscripten when I can find some more time!

faressc commented 7 months ago

Actually, I must be running into the same linker errors as you. I am using the script 

  python onnxruntime/tools/ci_build/build.py \
  --build_dir "onnxruntime/build/linux_${ARCH}" \
  --config=MinSizeRel \
  --parallel \
  --compile_no_warning_as_error \
  --skip_submodule_sync \
  --skip_tests \
  --minimal_build \
  --disable_ml_ops --disable_rtti \
  --include_ops_by_config "$ONNX_CONFIG" \
  --enable_reduced_operator_type_support \

Then I use

 ar -M << EOM                                                                               
    CREATE "output.a"
    ADDLIB "./libonnxruntime_graph.a"
    ADDLIB ...
    SAVE
    END
EOM

The linker errors I get are of the type

 undefined reference to `onnx::ModelProto::ModelProto(google::protobuf::Arena*, bool)'.
 undefined reference to `onnx::ModelProto::~ModelProto()'.
 # and more...

Do you get similar errors?

faressc commented 7 months ago

I just figured out the problem... When I was building onnxruntime, I had automatically linked against my system-wide installation of the protobuf library. When I then linked against the combined static onnxruntime library, that part of the library was missing...