Fix windows linking (#104)

* Fix finding of OpenVINO 2024.0.0 on Windows.

* Fixed linking on Windows.

* Fix Linux compile error.

* fix clippy error

* Refactor to pass linkage kind as a parameter

---------

Co-authored-by: Bradley Odell <[email protected]>
Co-authored-by: Andrew Brown <[email protected]>

Author: 3 people

.github/workflows/main.yml

@@ -63,9 +63,6 @@ jobs:
     # path and Cargo links the binary to it in the `build.rs` script.
     - name: Check dynamic linking
       run: cargo test
-      # TODO: the finder does not yet know how to distinguish between `*.lib` (required here) and
-      # `*.dll` (required by runtime-linking).
-      if: ${{ !startsWith(runner.os, 'windows') }}
     # Finally, run the runtime-linking tests: the binddings do not link at build time, instead
     # as the tests are run.
     - name: Check runtime linking

crates/openvino-finder/src/lib.rs

@@ -56,6 +56,22 @@ macro_rules! check_and_return {
     };
 }
 
+/// Distinguish which kind of library to link to.
+///
+/// The difference is important on Windows, e.g., which [requires] `*.lib` libraries when linking
+/// dependent libraries.
+///
+/// [requires]:
+///     https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-creation#creating-an-import-library
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum Linking {
+    /// Find _static_ libraries: OpenVINO comes with static libraries on some platforms (e.g.,
+    /// Windows).
+    Static,
+    /// Find _dynamic_ libraries.
+    Dynamic,
+}
+
 /// Find the path to an OpenVINO library.
 ///
 /// Because OpenVINO can be installed in quite a few ways (see module documentation), this function
@@ -87,13 +103,19 @@ macro_rules! check_and_return {
 /// # Panics
 ///
 /// Panics if it cannot list the contents of a search directory.
-pub fn find(library_name: &str) -> Option<PathBuf> {
-    let file = format!(
-        "{}{}{}",
-        env::consts::DLL_PREFIX,
-        library_name,
+pub fn find(library_name: &str, kind: Linking) -> Option<PathBuf> {
+    let suffix = if kind == Linking::Static {
+        // This is a bit rudimentary but works for the top three supported platforms: `linux`,
+        // `macos`, and `windows`.
+        if cfg!(target_os = "windows") {
+            ".lib"
+        } else {
+            ".a"
+        }
+    } else {
         env::consts::DLL_SUFFIX
-    );
+    };
+    let file = format!("{}{}{}", env::consts::DLL_PREFIX, library_name, suffix);
     log::info!("Attempting to find library: {}", file);
 
     // Search using the `OPENVINO_BUILD_DIR` environment variable; this may be set by users of the
@@ -220,6 +242,7 @@ const KNOWN_INSTALLATION_SUBDIRECTORIES: &[&str] = &[
     "runtime/lib/intel64/Release",
     "runtime/lib/intel64",
     "runtime/3rdparty/tbb/lib",
+    "runtime/bin/intel64/Release",
     "runtime/bin/intel64",
     "runtime/3rdparty/tbb/bin",
 ];
@@ -259,7 +282,7 @@ pub fn find_plugins_xml() -> Option<PathBuf> {
 
     // Check in the same directory as the `openvino_c` library; e.g.,
     // `/opt/intel/openvino_.../runtime/lib/intel64/plugins.xml`.
-    let library = find("openvino_c")?;
+    let library = find("openvino_c", Linking::Dynamic)?;
     let library_parent_dir = library.parent()?;
     check_and_return!(library_parent_dir.join(FILE_NAME));
 
@@ -314,7 +337,7 @@ mod test {
     #[test]
     fn find_openvino_c_locally() {
         env_logger::init();
-        assert!(find("openvino_c").is_some());
+        assert!(find("openvino_c", Linking::Dynamic).is_some());
     }
 
     /// This test shows how the finder would discover the latest shared library on an

crates/openvino-sys/build.rs

@@ -46,10 +46,13 @@ fn main() {
     };
 
     // Find the OpenVINO libraries to link to, either from a pre-installed location or by building
-    // from source.
+    // from source. We always look for the dynamic libraries here.
+    let link_kind = openvino_finder::Linking::Dynamic;
     let (c_api_library_path, library_search_paths) = if linking == Linking::None {
-        (openvino_finder::find("openvino_c"), vec![])
-    } else if let Some(path) = openvino_finder::find("openvino_c") {
+        // Why try to find the library if we're not going to link against it? Well, this is for the
+        // helpful Cargo warnings that get printed below if we can't find the library on the system.
+        (openvino_finder::find("openvino_c", link_kind), vec![])
+    } else if let Some(path) = openvino_finder::find("openvino_c", link_kind) {
         (Some(path), find_libraries_in_existing_installation())
     } else {
         panic!("Unable to find an OpenVINO installation on your system; build with runtime linking using `--features runtime-linking` or build from source with `OPENVINO_BUILD_DIR`.")
@@ -138,8 +141,16 @@ fn add_dynamically_linked_library(library: &str) {
 ///    unclear how we would discover this in a system-install scenario.
 fn find_libraries_in_existing_installation() -> Vec<PathBuf> {
     let mut dirs = vec![];
+    let link_kind = if cfg!(target_os = "windows") {
+        // Retrieve `*.lib` files on Windows. This is important because, when linking, Windows
+        // expects `*.lib` files. See
+        // https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-creation#creating-an-import-library.
+        openvino_finder::Linking::Static
+    } else {
+        openvino_finder::Linking::Dynamic
+    };
     for library in LIBRARIES {
-        if let Some(path) = openvino_finder::find(library) {
+        if let Some(path) = openvino_finder::find(library, link_kind) {
             println!(
                 "cargo:warning=Found library to link against: {}",
                 path.display()

crates/openvino-sys/src/lib.rs

@@ -62,7 +62,7 @@ pub mod library {
     /// `find().unwrap().parent()`.
     pub fn find() -> Option<PathBuf> {
         if cfg!(feature = "runtime-linking") {
-            openvino_finder::find("openvino_c")
+            openvino_finder::find("openvino_c", openvino_finder::Linking::Dynamic)
         } else {
             Some(PathBuf::from(env!("OPENVINO_LIB_PATH")))
         }