fpagliughi / rust-industrial-io

Rust interface to the Linux Industrial I/O subsystem
MIT License
45 stars 21 forks source link

Improved library detection and binding generation #30

Open ktkaufman03 opened 9 months ago

ktkaufman03 commented 9 months ago

This PR removes the version-specific features and bindings from libiio and libiio-sys, and provides a new build script that uses pkg-config to locate the native libiio (versions 0.19 through 0.25) on the system. bindgen is then used to generate bindings from the system-wide header.

I've marked this PR as a draft because it currently causes CI failures due to libiio not being installed in the runner. I'm open to suggestions for ways to handle this. My first thought was to simply install a specific version (or maybe multiple versions) of libiio as part of the CI process, but there may be a better approach.

fpagliughi commented 9 months ago

Hey, thanks for the PR!

I think there are a few things to keep in mind for this crate:

So I think some combination of what you propose and what's already here would be optimal.

First, it would be cool to be able to create the bindings, but only as an option. Something like:

$ cargo build --features=build_bindgen

similar to what I did in the Paho MQTT client crate: https://github.com/eclipse/paho.mqtt.rust/blob/a8618882938e813483ed04b621a90295f1974661/paho-mqtt-sys/build.rs#L128-L222

Running bindgen in the build is painfully slow, and produces the same output every time given the same header. Why re-run it every time you do a clean build? Especially if you're building natively on a small board because you don't have access to the other C libs for the target that your app needs to build. So I definitely like the pre-generated bindings for compile time.

The lesson I've learned with pre-generated bindings, though, is that you at least need to match the word size (64-bit vs 32-bit) for the intended target to avoid segfaults.

But you're right in that it would be nice, when you're building natively on Linux, for the default build to try and figure out what version of IIO is installed on the machine. But it would be cool for it to determine the version and then use the pre-generated bindings for that version. And then maybe just default to the latest supported version if it can't find the installed library header.

When you are cross-compiling, the IIO version on the target may not be the same as on your build host. And you may not even have the cross-compiled library on your build host. So it's nice to be able to specify the version of IIO on the target board. I can't give that up!

And finally, you may want to build this as a network client on a non-Linux system. Someone got it working for macOS (see #17 and #18), and theoretically it should work for a Windows client as well. It seems you removed that? Does your solution still work on a Mac then?

ktkaufman03 commented 9 months ago

My rationale for removing the Mac-specific code was that it didn't seem to be doing anything special other than setting the library search path. pkg-config handles that internally, so it should just work, but I would need to test it. Same deal with Windows - if you can get everything configured properly ahead of time (and I've checked; the pkg-config crate supports Windows), it should just work. Testing on Windows is significantly easier for me than testing on Mac, so I can take care of that.

That being said, your point about cross compilation is a good one. I'm just not sure if having premade bindings is really the right solution. Logically - at least to me - if you're compiling against a specific version of a library, you should have the library available on the build machine. But I also know that we don't need to overcomplicate things, so I'm happy to reintroduce the pre-made bindings if you think it's the best move.

The issue with bindgen being re-run every time is also fixable.

So, how about this: you either choose a specific version (via a feature) or allow the build script to generate bindings from whatever you have installed, for the appropriate architecture.

If that works, I can update my branch and we can work from there.

Thanks!

Get Outlook for iOShttps://aka.ms/o0ukef


From: Frank Pagliughi @.> Sent: Tuesday, March 5, 2024 1:26:53 AM To: fpagliughi/rust-industrial-io @.> Cc: Kaufman, Kai @.>; Author @.> Subject: [EXT] Re: [fpagliughi/rust-industrial-io] Improved library detection and binding generation (PR #30)

Hey, thanks for the PR!

I think there are a few things to keep in mind for this crate:

So I think some combination of what you propose and what's already here would be optimal.

First, it would be cool to be able to create the bindings, but only as an option. Something like:

$ cargo build --features=build_bindgen

similar to what I did in the Paho MQTT client crate: https://github.com/eclipse/paho.mqtt.rust/blob/a8618882938e813483ed04b621a90295f1974661/paho-mqtt-sys/build.rs#L128-L222

Running bindgen in the build is painfully slow, and produces the same output every time given the same header. Why re-run it every time you do a clean build? Especially if you're building natively on a small board because you don't have access to the other C libs for the target that your app needs to build. So I definitely like the pre-generated bindings for compile time.

The lesson I've learned with pre-generated bindings, though, is that you at least need to match the word size (64-bit vs 32-bit) for the intended target to avoid segfaults.

But you're right in that it would be nice, when you're building natively on Linux, for the default build to try and figure out what version of IIO is installed on the machine. But it would be cool for it to determine the version and then use the pre-generated bindings for that version. And then maybe just default to the latest supported version if it can't find the installed library header.

When you are cross-compiling, the IIO version on the target may not be the same as on your build host. And you may not even have the cross-compiled library on your build host. So it's nice to be able to specify the version of IIO on the target board. I can't give that up!

And finally, you may want to build this as a network client on a non-Linux system. Someone got it working for macOS (see #17https://github.com/fpagliughi/rust-industrial-io/pull/17 and #18https://github.com/fpagliughi/rust-industrial-io/issues/18), and theoretically it should work for a Windows client as well. It seems you removed that? Does your solution still work on a Mac then?

— Reply to this email directly, view it on GitHubhttps://github.com/fpagliughi/rust-industrial-io/pull/30#issuecomment-1978048489, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AWE3VFTGE7LKOFIPHMNTBRTYWVQS3AVCNFSM6AAAAABEGNAVGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZYGA2DQNBYHE. You are receiving this because you authored the thread.Message ID: @.***>

fpagliughi commented 9 months ago

the Mac-specific [...] didn't seem to be doing anything special other than setting the library search path. pkg-config handles that internally,

I have a Mac, but I don't use it that much, so I'm not too experienced with it. Is it common to have pkg-config installed on a Ma? Is that a safe assumption that we can use it without requiring users to install it just to include this one obscure Rust library in a build? Or, more to the point, should we break a (seemingly) working solution to require an external build dependency? That wouldn't make sense.

Ditto for Windows. I know that I don't have pkg-config on any of my Windows machines.

That said, it could definitely be useful if it's already installed. But if it isn't installed or fails for any reason, the build should attempt to recover. For example, try to use it, then, if it isn't installed look at the target OS; if we're targeting a Mac, then try the directories we're already using. Something similar for Windows.

Does that make sense?

ktkaufman03 commented 9 months ago

Sorry for the late response.

Is it common to have pkg-config installed on a Ma? Is that a safe assumption that we can use it without requiring users to install it just to include this one obscure Rust library in a build?

I don't think it's uncommon. It's been a while since I used a Mac as my daily driver, but when I did, I definitely had pkg-config installed. It's not exactly an obscure tool - a lot of things rely on it in one way or another.

Ditto for Windows. I know that I don't have pkg-config on any of my Windows machines.

I do, but it's only because I have w64devkit installed. This isn't totally unreasonable to expect in 2024, but it's still a potentially faulty assumption that could hurt... granted, this library (or at least, this version of the library) can't even be used on Windows at the moment, so maybe we can take a risk there.

For example, try to use it, then, if it isn't installed look at the target OS; if we're targeting a Mac, then try the directories we're already using.

The Mac-specific code was actually assuming (when running on ARM) that Homebrew was installed, which doesn't seem ideal. I don't know how many developers with ARM-based Macs are using this library, but I kind of doubt there are so many that it would be unreasonable to make any changes.

I made a diagram showing what I think is a reasonable flow for the build script to follow (with some minor details like caching removed, and some areas for discussion left in):

industrial-io build drawio

Let me know what you think.