-
Notifications
You must be signed in to change notification settings - Fork 619
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
switch to yeslogic-fontconfig-sys from servo-fontconfig #956
Conversation
internal/backends/gl/Cargo.toml
Outdated
# dlopen fontconfig at runtime rather than link at build time. | ||
# useful for cross compiling | ||
# Using a vendored fontconfig C library does not work well, refer to Issue #88. | ||
fontconfig-dlopen = [ "yeslogic-fontconfig-sys/dlopen" ] |
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.
IMHO we could just enable this by default, not sure we need it to be a feature at the moment.
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.
I discussed this with Fedora packagers recently with regard to using dlib for the JACK Rust bindings. They said they would prefer Rust packages to link C libraries. So I think it should be a feature. I lean towards leaving it off (linking) by default to ensure Slint applications comply with LInux packaging guidelines, but I don't have a strong opinion about whether the default is dlopen or linking.
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.
I think we can just let it the default for now.
If this turns out to be a problem, we can add features later.
Ideally, could it be the other way in yeslogic-fontconfig-sys? dlopen the default, and a feature to do the linking?
So downstream users can enable direct linking by adding a feature without having to propagate the feature through the whole stack.
Another option than a feature would be to add the ability to configure/override that with an environment variable read by the build script of the -sys crate.
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.
I like the idea to use an environment variable. build.rs could read an environment variable to toggle the Cargo feature via printing cargo:rustc-cfg=feature="dlopen". I agree that's a nicer solution than propagating the Cargo feature selection all the way down the stack of Cargo.toml's.
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.
Excellent point, I think the majority of users would probably want dlopen and the packaging might be the exception here. So opting into linkage might be easier.
Regarding packaging, I hope that if Slint gets packaged for a Linux distro it would be with the Qt backend instead of the GL backend.
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.
I think the fontconfig bindings should default to doing the right thing for packaging (linking, not dlopen). In practice I don't think it's a big issue to link at build time. If a developer of a Slint application has the build fail, the error message says why (pkgconfig couldn't find fontconfig), so it's not so hard to resolve. The confusion in #88 arose because servo-fontconfig silently did a bad thing (building a vendored C library) instead of failing the build when the fontconfig-dev package wasn't installed.
I don't expect packagers to read the documentation of all the dependencies of an application they're packaging. I also don't expect application developers to proactively communicate to packagers about a need to set an environment variable for a dependency. This PR solves a problem for cross compiling from Linux to Linux on a different CPU architecture, whereas I think most developers building Slint applications are going to building to run locally. I think it's fine to require setting an environment variable for the cross compiling case.
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.
I implemented setting the RUST_FONTCONFIG_DLOPEN=on
environment variable and removed exposing the Cargo feature down Slint's stack of Cargo.toml's.
f9d8993
to
b35a398
Compare
8ccd2d4
to
d9aa1f6
Compare
d9aa1f6
to
43f6256
Compare
internal/backends/gl/build.rs
Outdated
fn main() { | ||
let dlopen = std::env::var("RUST_FONTCONFIG_DLOPEN").is_ok(); | ||
if dlopen { | ||
println!("cargo:rustc-cfg=feature=\"fontconfig-dlopen\""); |
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 won't work. It's not possible to change the features from a build script.
Also build script are run after the dependencies nare build.
This logic would have to go in the build script of yes-confondus
fontconfig
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.
It does work, but it requires this in the build.rs of both yeslogic-fontconfig-sys and here because both have conditional compilation depending on a Cargo feature. You are correct that Cargo determines dependencies before build.rs is run, which means that the feature activated here cannot enable an optional dependency: yeslogic/fontconfig-rs#12 (comment)
43f6256
to
7e305fc
Compare
7f7eb95
to
0c287ce
Compare
The PR for font-kit was finally merged. I'll make a PR for plotters soon. |
Yay, that's good news! |
PR for plotters: plotters-rs/plotters#337 |
956972e
to
f211390
Compare
I am puzzled by the build errors on CI for font-kit. I cannot reproduce them locally.
|
7432f6b
to
11c6d4a
Compare
I figured out the compile error. The combination of
I resolved this by adding a
|
11c6d4a
to
041b8c8
Compare
082e2d0
to
2642667
Compare
Using this branch, I got my application to cross compile to ARM64 on Fedora without even using Cross or a container. I had to install a cross toolchain with
then it just worked with
|
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.
Thanks for working on this.
I have the feeling that dlopen should actually be the default.
What we want is for user that try to use cargo run or cargo build that it "just works" and dlopen sound to me like the best approach.
I know that packagers don't like it, but they can play with compilation flags
I think that would be difficult to change at this point. build.rs behaves the same way using the same
They can't with the way it is now because the build.rs's mentioned above only check if the environment variable is set. There is not a way to disable |
2642667
to
2ce72c5
Compare
I discussed this with @decathorpe who does a lot of Rust packaging for Fedora and his opinion is that linking should generally be the default. If Slint can work without fontconfig, it could be okay to make |
20142fb
to
00672b2
Compare
7f4be22
to
0d8643e
Compare
This allows setting the RUST_FONTCONFIG_DLOPEN environment variable to dlopen fontconfig at runtime rather than linking it at build time. This is helpful for cross compiling to Linux, particularly because fontconfig has lots of C dependencies. Building a vendored copy of fontconfig does not work as expected: slint-ui#88
ec90fa1
to
8a824d5
Compare
Simpler solution: don't expose |
Rebased and CI passed. Let's get this merged and not let it bitrot again. |
println!("cargo:rerun-if-env-changed=RUST_FONTCONFIG_DLOPEN"); | ||
let dlopen = std::env::var("RUST_FONTCONFIG_DLOPEN").is_ok(); | ||
if dlopen { | ||
println!("cargo:rustc-cfg=feature=\"fontconfig-dlopen\""); |
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 should be a feature, just a plain cfg.
Also, could we have the default reversed? Default to dlopen and have a env variable to disable 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.
This should be a feature, just a plain cfg.
I'm not sure what you mean. Listing it as a feature in Cargo.toml? I could do that, but it requires doing that in every crate that uses fontconfig for building the workspace with --all-features
to work.
Also, could we have the default reversed? Default to dlopen and have a env variable to disable it?
Technically possible, but that would require changing how the build scripts handle the environment variable all up and down the stack (yeslogic-fontconfig-sys, fontconfig, fontkit, plotters, Slint crates) which I'd rather not deal with.
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.
I mean println!("cargo:rustc-cfg=fontconfig-dlopen");
. Because i believe the feature=
is to be used with cargo features.
Unless i'm missing something. What other crate are using that.
Technically possible, but that would require changing how the build scripts handle the environment variable all up and down the stack (yeslogic-fontconfig-sys, fontconfig, fontkit, plotters, Slint crates) which I'd rather not deal with.
I see. This is unfortunate.
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.
Yes, feature=
is intended to be used with Cargo features. It is unusual for build scripts to print rustc-cfg=feature
. This has the limitation of not being able to enable optional dependencies because Cargo.toml is parsed before build scripts are run, but that's not relevant in this case. Also, feature=
is required for dlib::ffi_dispatch!.
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.
Just one question within to see if I understand this correctly. I'm personally not a big fan of environment variables for toggling these kind of features, but my opinion is not relevant: This was accepted upstream by the fontconfig crate and having this opt-in feature is an improvement for sure.
`xcb` and `xcbcommon` are not needed if you are only using `backend-winit-wayland` without `backend-winit-x11`. | ||
|
||
fontconfig can be `dlopen`ed at runtime instead of linking it at build time by setting the | ||
environment variable `RUST_FONTCONFIG_DLOPEN=on`. This can be useful for [cross-compiling](#cross-compiling). |
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 my understanding correct that RUST_FONTCONFIG_DLOPEN
is the name of the environment variable that's not only used by Slint's build script but also by yeslogic-fontconfig itself?
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.
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.
FWIW, if Slint used the safe fontconfig
wrapper around yeslogic-fontconfig-sys
, the build.rs scripts in Slint wouldn't be necessary.
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.
Ohh, that looks new/updated and promising. I might look into that separately unless somebody beats me to it. But then I think this merge request can go in - yay :)
to use
dlopen
for fontconfig instead of linking fontconfig at build time. This allows for easier cross compilation, which I am using to cross compile from x86_64-unknown-linux-gnu to aarch64-unknown-linux-gnu with cross:Blocked by: