Cross compile fixes #153

madwizard-thomas · 2023-11-14T20:49:30Z

Fixes #102

When cross-compiling this library (in my case for ESP32 but the fixes are generic) I came across several issues. This pull requests should fix these.

lvgl-sys is built twice, once as a normal dependency for lvgl (for the application) and once as build dependency (for the build script executable of lvgl crate). The normal dependency includes the automatically generated bindings (built by bindgen) and links the lvgl C library itself (using the cc crate). The build dependency is used to call _bindgen_raw_src to retrieve the raw source code of the generated bindings to further autogenerate rust code.

Both the library and the bindings are generated twice, for the application and build script. For the application the target (TARGET env) is set correctly, however the build script is a host targeted executable (TARGET == HOST), so the library and bindings are actually generated for the host machine. The library itself is not used for the build executable so this problem is silent, it does take unnecessary time to compile though. The bindings however are used in the build host executable, but here the bindings should be compiled for the target machine and not for the host. In most cases these bindings will be the same but there may be subtle differences leading to bugs. For both the application and the build script, the bindings need to be targeted to the embedded device.

Since HOST == TARGET in the build script, there is no way to tell the actual application target. The cc crate allows the environment variable CROSS_COMPILE to be set to specify a different compiler. It is only used by cc when host != target though. Bindgen does not use this variable, also it needs to cross-compile even when host == target so in this case target should always be CROSS_COMPILE.

While figuring this out I also came across a slightly unrelated issue with this line in lvlg-sys build.rs:
add_c_files(&mut cfg, &lv_config_dir);
This actually compiles all c files recursively starting at the config dir. Since the config dir is often the project dir this causes the script to possibly build all kinds of unrelated files. Especially with esp32 which stores the c source of esp-idf in a some directory in the project. I removed this line since the documentation says nothing about compiling C files from the config dir. If this is required for someone it should probably be a separate env var.

This PR consists of 3 commits:
Remove compilation of all recursive C files in config dir
Removes the add_c_files line for the config and adds a missing rerun-if-env-changed for the config env var.

Refactor build script: separate library and bindings in separate functions
Lots of changed lines but actually changes no functionality, just refactors the build script in two separate steps and cleans it up a bit

Add library and raw-bindings features to lvgl-sys
The library feature switches the compilation (cc) of the library on or off
The raw-bindings feature toggles _bindgen_raw_src

The normal dependency on lvgl-sys enables the library feature: library and bindings compiled (cc+bindgen) for the target application, no raw bindings needed
The build dependency enables only raw-bindings: library not compiled (no cc), raw bindings generated for build step
generate_bindings is modified to prefer CROSS_COMPILE over TARGET in all cases, so that bindings are generated for the correct target in both the build and application step.

This fixes the build for ESP32 (checked that it at least compiles for no-std and std esp templates) and it still works on linux with SDL (tested).

For ESP32 (and other embedded targets), the CROSS_COMPILE variable needs to be set, for example for esp32s3 you would need export CROSS_COMPILE=xtensa-esp32s3-elf, or better include it as [env] in .cargo/config.toml
Another issue is that bindgen (at least for ESP32) does not have a sysroot by default. This can be set using export BINDGEN_EXTRA_CLANG_ARGS="--sysroot directoryhere". To find the correct sysroot you can run one of the gcc compiler tools with --print-sysroot, though cc does not work so use ld for example: xtensa-esp32s3-elf-ld --print-sysroot.

Or if CROSS_COMPILE is available in one go:

export CROSS_COMPILE=xtensa-esp32s3-elf
export BINDGEN_EXTRA_CLANG_ARGS="--sysroot "`$CROSS_COMPILE-ld --print-sysroot`

The bindgen sysroot could be done by the build.rs script as well but maybe that's a bit too specific.

Add missing rerun-if-env-changed for config path env var

…ions

Correct CROSS_COMPILE target for bindgen

enelson1001 · 2023-11-28T22:00:07Z

@madwizard-thomas I cloned the lv_binding_rust and applied your changes (At least I think I have all the changes incorporated). Everything builds and applications runs fine. The only thing I noticed was I get 2 sets of the same warnings which makes me think something is running twice. This is what I see during the cargo build.

 Compiling lvgl v0.6.2 (/home/ed/lv-binding-rust/lv_binding_rust_patch_madwizard/lvgl)
    Finished dev [optimized + debuginfo] target(s) in 2m 48s
warning: `extern` block uses type `[u32; 3]`, which is not FFI-safe
    --> /home/ed/esp-rust-projects/aliexpress-esp32s3/rust-esp32s3-lvgl-clickme/target/debug/build/lvgl-sys-23f05043d1aabbc6/out/bindings.rs:1482:13
     |
1482 |         va: va_list,
     |             ^^^^^^^ not FFI-safe
     |
     = help: consider passing a pointer to the array
     = note: passing raw arrays by value is not FFI-safe
     = note: `#[warn(improper_ctypes)]` on by default

warning: `extern` block uses type `[u32; 3]`, which is not FFI-safe
    --> /home/ed/esp-rust-projects/aliexpress-esp32s3/rust-esp32s3-lvgl-clickme/target/debug/build/lvgl-sys-23f05043d1aabbc6/out/bindings.rs:2223:63
     |
2223 |     pub fn _lv_txt_set_text_vfmt(fmt: *const cty::c_char, ap: va_list) -> *mut cty::c_char;
     |                                                               ^^^^^^^ not FFI-safe
     |
     = help: consider passing a pointer to the array
     = note: passing raw arrays by value is not FFI-safe

warning: 2 warnings emitted

warning: `extern` block uses type `[u32; 3]`, which is not FFI-safe
    --> /home/ed/esp-rust-projects/aliexpress-esp32s3/rust-esp32s3-lvgl-clickme/target/xtensa-esp32s3-espidf/debug/build/lvgl-sys-c6306817d27c4c31/out/bindings.rs:1482:13
     |
1482 |         va: va_list,
     |             ^^^^^^^ not FFI-safe
     |
     = help: consider passing a pointer to the array
     = note: passing raw arrays by value is not FFI-safe
     = note: `#[warn(improper_ctypes)]` on by default

warning: `extern` block uses type `[u32; 3]`, which is not FFI-safe
    --> /home/ed/esp-rust-projects/aliexpress-esp32s3/rust-esp32s3-lvgl-clickme/target/xtensa-esp32s3-espidf/debug/build/lvgl-sys-c6306817d27c4c31/out/bindings.rs:2223:63
     |
2223 |     pub fn _lv_txt_set_text_vfmt(fmt: *const cty::c_char, ap: va_list) -> *mut cty::c_char;
     |                                                               ^^^^^^^ not FFI-safe
     |
     = help: consider passing a pointer to the array
     = note: passing raw arrays by value is not FFI-safe

warning: 2 warnings emitted

Is this what you saw when you ran your tests?

madwizard-thomas · 2023-11-29T14:26:09Z

Yes, it generates the bindings twice, once for the host target (build.rs executable) and once for the embedded target. It needs them in both, I don't think there is a way around it.
You can identify them by the target directories btw, target/debug -> host executable, target/xtensta-.. -> embedded target

nia-e

Looks lovely! There's a few nits/questions, other than that feel free to cargo fmt, and I'll give it a test & merge if all is fine :D

nia-e · 2023-11-29T14:41:20Z

lvgl/Cargo.toml

@@ -12,7 +12,7 @@ keywords = ["littlevgl", "lvgl", "graphical_interfaces"]
 build = "build.rs"

 [dependencies]
-lvgl-sys = { version = "0.6.2", path = "../lvgl-sys" }
+lvgl-sys = { version = "0.6.2", path = "../lvgl-sys", features = ["library"]}


Tiny nit; here (and line 80) add a space before the curly brace for stylistic consistency

nia-e · 2023-11-29T14:44:00Z

lvgl-sys/build.rs

-    let target = env::var("TARGET").expect("Cargo build scripts always have TARGET");
+    let target = env::var("CROSS_COMPILE").map_or_else(
+        |_| env::var("TARGET").expect("Cargo build scripts always have TARGET"),
+        |c| c.trim_end_matches('-').to_owned(),


This may be set incorrectly or by some other process (or accidentally left in a shell); may be worth falling back to the TARGET var if compile fails with this and warn

nia-e · 2023-11-29T14:45:39Z

lvgl/Cargo.toml

@@ -77,7 +77,7 @@ unsafe_no_autoinit = []
 quote = "1.0.23"
 proc-macro2 = "1.0.51"
 lvgl-codegen = { version = "0.6.2", path = "../lvgl-codegen" }
-lvgl-sys = { version = "0.6.2", path = "../lvgl-sys" }
+lvgl-sys = { version = "0.6.2", path = "../lvgl-sys", features = ["raw-bindings"]}


v2 of the cargo feature resolver does not unify features on crates compiled multiple times. If this and the above both specify both features for lvgl-sys (instead of just 1 each), it should unify and so the double-warnings might disappear?

That defeats the entire purpose of this PR, which is to prevent building the library for lvgl's build.rs executable
The library will always be built twice, since one is compiled for xtensa (or riscv), the other for x86_64. So there is no way to unify that.

Although this does only apply to cross-compilation, if target == host (no cross compile) it may compile it twice unnecessarily as well.

Why not check for that in the build script & enable features from there by printlning cargo:rustc-cfg=whatever? Beyond TARGET, cargo also exports HOST, so a simple check together with the previous one might work (on this, maybe just check if those are the same instead? I wasn't able to find any docs on a CROSS_COMPILE variable)

When lvgl-sys is compiled for the build script executable, target is always the same as host (since build scripts must run on the host). So in this case there is no way to find out the actual target. CROSS_COMPILE is just some standard used, the cc crate supports it so I thought it made sense to use it for the bindings.

Can we gate this behind a flag or feature, then, so that we don't always compile twice? Feels like a pretty big cost still.

nia-e · 2023-11-29T19:10:59Z

I am going to try to make it so only one version gets built in the first place - should be doable I think, possibly by splitting off another intermediary crate? This current approach is definitely "wrong" in that if C LVGL headers are materially different when cross-compiled it's going to just fail, but that's a more long term project once I get back to this proper. Until then this is very, very welcome

madwizard-thomas · 2023-11-29T20:33:01Z

I think you can do that if you split the binding and compile features into separate crates. Then you don’t need the features anymore.
You probably would need to always add the raw bindings function as well, even for the embedded target library though hopefully it will be removed from the final binary (this is the library’s current situation anyway)

When cross compiling I think it will always need to generate the bindings twice since there the architectures are different.

madwizard-thomas added 3 commits November 13, 2023 22:00

Remove compilation of all recursive C files in config dir

71f3518

Add missing rerun-if-env-changed for config path env var

Refactor build script: separate library and indings in separate funct…

aa88305

…ions

Add library and raw-bindings features to lvgl-sys.

4f3fdd8

Correct CROSS_COMPILE target for bindgen

nia-e reviewed Nov 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross compile fixes #153

Cross compile fixes #153

madwizard-thomas commented Nov 14, 2023

enelson1001 commented Nov 28, 2023

madwizard-thomas commented Nov 29, 2023

nia-e left a comment

nia-e Nov 29, 2023

nia-e Nov 29, 2023

nia-e Nov 29, 2023

madwizard-thomas Nov 29, 2023 •

edited

madwizard-thomas Nov 29, 2023

nia-e Nov 29, 2023

madwizard-thomas Nov 29, 2023

nia-e Dec 1, 2023

nia-e commented Nov 29, 2023 •

edited

madwizard-thomas commented Nov 29, 2023 •

edited

Cross compile fixes #153

Are you sure you want to change the base?

Cross compile fixes #153

Conversation

madwizard-thomas commented Nov 14, 2023

enelson1001 commented Nov 28, 2023

madwizard-thomas commented Nov 29, 2023

nia-e left a comment

Choose a reason for hiding this comment

nia-e Nov 29, 2023

Choose a reason for hiding this comment

nia-e Nov 29, 2023

Choose a reason for hiding this comment

nia-e Nov 29, 2023

Choose a reason for hiding this comment

madwizard-thomas Nov 29, 2023 • edited

Choose a reason for hiding this comment

madwizard-thomas Nov 29, 2023

Choose a reason for hiding this comment

nia-e Nov 29, 2023

Choose a reason for hiding this comment

madwizard-thomas Nov 29, 2023

Choose a reason for hiding this comment

nia-e Dec 1, 2023

Choose a reason for hiding this comment

nia-e commented Nov 29, 2023 • edited

madwizard-thomas commented Nov 29, 2023 • edited

madwizard-thomas Nov 29, 2023 •

edited

nia-e commented Nov 29, 2023 •

edited

madwizard-thomas commented Nov 29, 2023 •

edited