Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix windows linking #104

Merged
merged 6 commits into from
May 16, 2024
Merged

Fix windows linking #104

merged 6 commits into from
May 16, 2024

Conversation

rahulchaphalkar
Copy link
Contributor

  • Fix finding of OpenVINO 2024.0.0 on Windows
  • Fix linking Windows

Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I have a couple of thoughts here before we merge this:

  • openvino-finder API: this crate is already very confusing and I'm concerned that exposing find_link is going to make it more so. It's not the PRs fault: it's mainly that loading these libraries from the many locations OpenVINO stores them in is quite tricky and the terms that different operating systems use to distinguish between static, dynamic, and runtime linking are all different. So I think we need to add the handling for *.lib files but I think we should expand the definition of find() with an extra parameter that distinguishes between static and dynamic libraries. @rahulchaphalkar, if you allow me to push to this branch I can do that expansion and document what is going on here.
  • supported versions: the more I mull it over, the more easily I think, "let's drop the older versions of OpenVINO" from the CI matrix. Those Windows libraries apparently don't have ov_core_read_model_from_memory_buffer and making things work for the deprecated ov_core_read_model_from_memory on certain versions is too much complexity. Users can stick with a v0.6.* version of the crate if they want to use OpenVINO v2023.1.0 and below. @rahulchaphalkar, I propose we (a) eliminate those older versions from the CI matrix and (b) include one "special" entry (like the APT one) for the oldest version we think should still work on Ubuntu as a marker point for what's possible. We should also include v2024.1.0 here since that is now released.

In the future, I think we need to figure out the macOS TODO, but that might be tricky.

Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I've modified @BTOdell's original code a bit but the idea remains the same. I think this should be good to merge.

@abrown abrown merged commit 836dd87 into intel:main May 16, 2024
16 checks passed
@abrown abrown deleted the fix-windows-linking branch May 16, 2024 00:42
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.

3 participants