-
Notifications
You must be signed in to change notification settings - Fork 70
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
Cross compile fixes #153
base: master
Are you sure you want to change the base?
Cross compile fixes #153
Conversation
Add missing rerun-if-env-changed for config path env var
Correct CROSS_COMPILE target for bindgen
@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.
Is this what you saw when you ran your tests? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit; here (and line 80) add a space before the curly brace for stylistic consistency
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this does only apply to cross-compilation, if target == host
(no cross compile) it may compile it twice unnecessarily as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check for that in the build script & enable features from there by println
ing 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only thing preventing this PR from landing?
If so, this can just have a no_cross_compile
feature that enables both library
and raw-bindings
features of the lvgl-sys
crate.
And this will apply to the normal dependency and the build dependency confirmed
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 |
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. When cross compiling I think it will always need to generate the bindings twice since there the architectures are different. |
Any chance this could land? |
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 forlvgl
(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 offThe
raw-bindings
feature toggles_bindgen_raw_src
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:
The bindgen sysroot could be done by the build.rs script as well but maybe that's a bit too specific.