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

v4l2-sys: Replace FreeBSD **host-only** include path override with docs #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Collaborator

@MarijnS95 MarijnS95 commented Sep 19, 2024

In #106 we were too focused on getting rid of a pkg-config dependency via an unused system library, that a cfg!() in build.rs slipped in. @vladmovchan can you help us check the XXX comment specifically? I think we should remove this workaround and instead require the caller to set the correct sysroot or include(s) via BINDGEN_EXTRA_CLANG_ARGS_<triple> to make sure cross-compilations works to and from FreeBSD. Done!


With cfg!() on target_os this include path is unconditionally used if the host OS is FreeBSD, even if the target OS is different (and its cross-compilation headers are installed elsewhere on the system). The accurate target OS, regardless of what the build script is running on is stored in CARGO_CFG_TARGET_OS.

Since it is unlikely that the FreeBSD headers reside in /usr/ local/include when the target is FreeBSD while the host may be something completely different, remove the workaround and document how the user can set up arbitrary include directories for their target using BINDGEN_EXTRA_CLANG_ARGS (or the triple-specific variant) by documenting this environment variable in the main README. It is common for developers to maintain such a configuration in their home directory's ~/.cargo/ config.toml for the various architectures that they cross-compile to (together with related variables for the linker and cc-rs).

@vladmovchan
Copy link
Contributor

@MarijnS95, I actually didn't think about cross-compilation when we were working on the previous change.

I think you are right that CARGO_CFG_TARGET_OS environment variable should be used instead of cfg!(target_os = ...).
And I think you are correct that currently hardcoded "-I/usr/local/include" value is wrong in case of cross-compilation.

I agree with the solution you propose (removing existing workaround and relying-on/requiring BINDGEN_EXTRA_CLANG_ARGS_<triple>). I tried to find a better solution, but so far I didn't find any.

@MarijnS95
Copy link
Collaborator Author

@vladmovchan if you can confirm that this crate builds with BINDGEN_EXTRA_CLANG_ARGS_x86_64-unknown-freebsd=-I/usr/local/include/ before your PR(s), I'll remove the change and document this in a readme somewhere instead. Thanks for confirming!

@vladmovchan
Copy link
Contributor

vladmovchan commented Sep 20, 2024

I tried 6d17141 commit (the last commit before first PR has been merged) with minor additional change from master (which is required as libc::SYS_ioctl is not available on FreeBSD):

diff --git a/src/v4l2/api.rs b/src/v4l2/api.rs
index 0f6fb1d..19b834f 100644
--- a/src/v4l2/api.rs
+++ b/src/v4l2/api.rs
@@ -66,6 +66,7 @@ mod detail {
     pub unsafe fn close(fd: std::os::raw::c_int) -> std::os::raw::c_int {
         libc::close(fd)
     }
+    #[cfg(any(target_os = "linux", target_os = "android"))]
     pub unsafe fn ioctl(
         fd: std::os::raw::c_int,
         request: vidioc::_IOC_TYPE,
@@ -80,6 +81,14 @@ mod detail {
          */
         libc::syscall(libc::SYS_ioctl, fd, request, argp) as std::os::raw::c_int
     }
+    #[cfg(target_os = "freebsd")]
+    pub unsafe fn ioctl(
+        fd: std::os::raw::c_int,
+        request: vidioc::_IOC_TYPE,
+        argp: *mut std::os::raw::c_void,
+    ) -> std::os::raw::c_int {
+        libc::ioctl(fd, request, argp)
+    }
     pub unsafe fn mmap(
         start: *mut std::os::raw::c_void,
         length: usize,

Without any extra environment variables, it produces expected error:

wrapper.h:1:10: fatal error: 'linux/videodev2.h' file not found

With BINDGEN_EXTRA_CLANG_ARGS_x86_64_unknown_freebsd="-I/usr/local/include/" it compiles without any errors or warnings.

When you are going to add this to documentation, please use environment variable name with all underscores (BINDGEN_EXTRA_CLANG_ARGS_x86_64_unknown_freebsd), as the name with dashes (BINDGEN_EXTRA_CLANG_ARGS_x86_64-unknown-freebsd) is misinterpreted at least by bash and fish shells.

Thank you!

@MarijnS95 MarijnS95 changed the title v4l2-sys: Only use FreeBSD header include if the (cross-compilation) target OS is FreeBSD v4l2-sys: Replace FreeBSD **host-only** include path override with docs Oct 9, 2024
@MarijnS95 MarijnS95 marked this pull request as ready for review October 9, 2024 07:23
@MarijnS95
Copy link
Collaborator Author

All updated now, thanks for the wait and thanks for the test @vladmovchan! Let me know what you think about the new docs!

README.md Outdated Show resolved Hide resolved
@vladmovchan
Copy link
Contributor

It is a great explanation/documentation - I like it 👍

With `cfg!()` on `target_os` this include path is unconditionally used
if the _host_ OS is FreeBSD, even if the target OS is different (and its
cross-compilation headers are installed elsewhere on the system).  The
accurate target OS, regardless of what the build script is _running on_
is stored in `CARGO_CFG_TARGET_OS`.

Since it is unlikely that the FreeBSD headers reside in `/usr/
local/include` when the *target* is FreeBSD while the host may be
something completely different, remove the workaround and document
how the user can set up arbitrary include directories for their target
using `BINDGEN_EXTRA_CLANG_ARGS` (or the triple-specific variant) by
documenting this environment variable in the main `README`.  It is
common for developers to maintain such a configuration in their home
directory's `~/.cargo/ config.toml` for the various architectures that
they cross-compile to (together with related variables for the linker
and `cc-rs`).
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.

2 participants