-
Notifications
You must be signed in to change notification settings - Fork 581
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,4 +11,10 @@ fn main() { | |
skia_backend_metal: { all(target_os = "macos", not(feature = "renderer-winit-skia-opengl")) }, | ||
skia_backend_d3d: { all(target_family = "windows", not(feature = "renderer-winit-skia-opengl")) }, | ||
} | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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
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 commentThe reason will be displayed to describe this comment to others. Learn more. I mean
I see. This is unfortunate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, |
||
} | ||
} |
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.
Correct: https://github.com/yeslogic/fontconfig-rs/blob/7c8cff480e73b4c55f215d20fd660489855b3057/fontconfig/build.rs
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 aroundyeslogic-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 :)