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

switch to yeslogic-fontconfig-sys from servo-fontconfig #956

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Feb 16, 2022

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:

image

Blocked by:

@Be-ing Be-ing marked this pull request as draft February 16, 2022 07:03
@CLAassistant
Copy link

CLAassistant commented Feb 16, 2022

CLA assistant check
All committers have signed the CLA.

# 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" ]
Copy link
Member

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.

Copy link
Contributor Author

@Be-ing Be-ing Feb 16, 2022

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@Be-ing Be-ing Feb 16, 2022

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.

Copy link
Contributor Author

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.

fn main() {
let dlopen = std::env::var("RUST_FONTCONFIG_DLOPEN").is_ok();
if dlopen {
println!("cargo:rustc-cfg=feature=\"fontconfig-dlopen\"");
Copy link
Member

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

Copy link
Contributor Author

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)

@Be-ing Be-ing force-pushed the fontconfig-dlopen branch 2 times, most recently from 7f7eb95 to 0c287ce Compare March 1, 2022 05:22
@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 28, 2022

The PR for font-kit was finally merged. I'll make a PR for plotters soon.

@tronical
Copy link
Member

Yay, that's good news!

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 29, 2022

PR for plotters: plotters-rs/plotters#337

@Be-ing Be-ing force-pushed the fontconfig-dlopen branch 2 times, most recently from 956972e to f211390 Compare March 31, 2022 02:09
@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 31, 2022

I am puzzled by the build errors on CI for font-kit. I cannot reproduce them locally.

error[E0425]: cannot find function, tuple struct or tuple variant `FcInitLoadConfigAndFonts` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:320:25
    |
320 |                         FcInitLoadConfigAndFonts,
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcConfigDestroy` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:333:21
    |
333 |                     FcConfigDestroy,
    |                     ^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcPatternCreate` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:359:21
    |
359 |                     FcPatternCreate,
    |                     ^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcNameParse` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:371:21
    |
371 |                     FcNameParse,
    |                     ^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcPatternAddString` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:384:21
    |
384 |                     FcPatternAddString,
    |                     ^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcConfigSubstitute` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:401:21
    |
401 |                     FcConfigSubstitute,
    |                     ^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcDefaultSubstitute` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:415:21
    |
415 |                     FcDefaultSubstitute,
    |                     ^^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcFontSort` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:428:21
    |
428 |                     FcFontSort,
    |                     ^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcFontList` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:452:21
    |
452 |                     FcFontList,
    |                     ^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcPatternDestroy` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:473:21
    |
473 |                     FcPatternDestroy,
    |                     ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcPatternGetString` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:493:21
    |
493 |                     FcPatternGetString,
    |                     ^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcPatternGetInteger` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:521:21
    |
521 |                     FcPatternGetInteger,
    |                     ^^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcFontSetDestroy` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:577:21
    |
577 |                     FcFontSetDestroy,
    |                     ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcObjectSetCreate` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:593:81
    |
593 |                     d: ffi_dispatch!(feature = "source-fontconfig-dlopen", LIB, FcObjectSetCreate,),
    |                                                                                 ^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcObjectSetAdd` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:607:25
    |
607 |                         FcObjectSetAdd,
    |                         ^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `FcObjectSetDestroy` in this scope
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:623:21
    |
623 |                     FcObjectSetDestroy,
    |                     ^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0699]: the type of this value must be known to call a method on a raw pointer on it
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/font-kit-0.11.0/src/sources/fontconfig.rs:503:27
    |
503 |                 if string.is_null() {
    |                           ^^^^^^^

@Be-ing Be-ing force-pushed the fontconfig-dlopen branch 3 times, most recently from 7432f6b to 11c6d4a Compare April 5, 2022 00:36
@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 5, 2022

I figured out the compile error. The combination of --workspace and --all-features as the CI uses and feature-resolver = 2 in the workspace's Cargo.toml created a strange situation. The plotter example was depending on yeslogic-fontconfig-sys with the dlopen feature because of i-slint-backend-gl and i-slint-compiler's fontconfig-dlopen features. However, plotters did not activate the source-fontconfig-dlopen feature of font-kit required for font-kit to work with yeslogic-fontconfig-sys's dlopen feature:

$ cargo tree --workspace -f "{p} {f}" -i yeslogic-fontconfig-sys --all-features

...

yeslogic-fontconfig-sys v3.0.0 dlopen
├── font-kit v0.11.0 default,source
│   └── plotters v0.3.1 (https://github.com/Be-ing/plotters.git?branch=font-kit-update#cc2c2ffd) bitmap_backend,font-kit,lazy_static,pathfinder_geometry,plotters-bitmap,surface_series,ttf,ttf-parser
│       └── plotter v0.2.2 (/home/be/sw/slint/examples/plotter) 
├── i-slint-backend-gl v0.2.2 (/home/be/sw/slint/internal/backends/gl) default,fontconfig-dlopen,resvg,rtti,svg,tiny-skia,usvg,wayland,x11 (*)
└── i-slint-compiler v0.2.2 (/home/be/sw/slint/internal/compiler) codemap,codemap-diagnostic,cpp,display-diagnostics,fontconfig-dlopen,proc-macro2,proc_macro_span,quote,rust
    ├── slint-build v0.2.2 (/home/be/sw/slint/api/rs/build) default
    │   [build-dependencies]
    │   ├── gallery v0.2.2 (/home/be/sw/slint/examples/gallery) 
    │   ├── imagefilter v0.2.2 (/home/be/sw/slint/examples/imagefilter) 
    │   ├── memory v0.2.2 (/home/be/sw/slint/examples/memory) 
    │   ├── opengl_underlay v0.2.2 (/home/be/sw/slint/examples/opengl_underlay) 
    │   ├── plotter v0.2.2 (/home/be/sw/slint/examples/plotter) 
    │   ├── printerdemo v0.2.2 (/home/be/sw/slint/examples/printerdemo/rust) 
    │   ├── printerdemo_mcu v0.2.2 (/home/be/sw/slint/examples/printerdemo_mcu) chrono,mcu-pico-st7789,mcu-simulator
    │   ├── printerdemo_old v0.2.2 (/home/be/sw/slint/examples/printerdemo_old/rust) 
    │   ├── slide_puzzle v0.2.2 (/home/be/sw/slint/examples/slide_puzzle) 
    │   └── todo v0.2.2 (/home/be/sw/slint/examples/todo/rust) 
    │   [dev-dependencies]
    │   └── slint v0.2.2 (/home/be/sw/slint/api/rs/slint) backend-gl-all,backend-gl-wayland,backend-gl-x11,backend-qt,compat-0-2-0,default,document-features,std (*)
    ├── slint-compiler v0.2.2 (/home/be/sw/slint/tools/compiler) 
    ├── slint-fmt v0.2.2 (/home/be/sw/slint/tools/fmt) 
    ├── slint-interpreter v0.2.2 (/home/be/sw/slint/internal/interpreter) backend-gl-all,backend-gl-wayland,backend-gl-x11,backend-qt,compat-0-2-0,default,display-diagnostics,document-features,ffi,spin_on,std (*)
    ├── slint-lsp v0.2.2 (/home/be/sw/slint/tools/lsp) backend-gl-all,backend-gl-wayland,backend-gl-x11,backend-qt,default
    ├── slint-node v0.2.2 (/home/be/sw/slint/api/node/native) 
    └── syntax_updater v0.2.2 (/home/be/sw/slint/tools/syntax_updater) 
    [dev-dependencies]
    ├── doctests v0.2.2 (/home/be/sw/slint/tests/doctests) 
    └── test-driver-cpp v0.2.2 (/home/be/sw/slint/tests/driver/cpp)

I resolved this by adding a fontconfig-dlopen feature to plotters to activate font-kit's source-fontconfig-dlopen feature in plotters-rs/plotters#337 then exposing it as a feature in the plotter example. This way --workspace --all-features activates font-kit's source-fontconfig-dlopen feature when the fontconfig-dlopen features of i-slint-backend-gl and i-slint-compiler are enabled by --all-features.

$ cargo tree --workspace -f "{p} {f}" -i yeslogic-fontconfig-sys --all-features

...

yeslogic-fontconfig-sys v3.0.1 dlopen
├── font-kit v0.11.0 default,source,source-fontconfig-dlopen,yeslogic-fontconfig-sys
│   └── plotters v0.3.1 (https://github.com/Be-ing/plotters.git?branch=font-kit-update#fec5dc7c) bitmap_backend,font-kit,fontconfig-dlopen,lazy_static,pathfinder_geometry,plotters-bitmap,surface_series,ttf,ttf-parser
│       └── plotter v0.2.2 (/home/be/sw/slint/examples/plotter) fontconfig-dlopen
├── i-slint-backend-gl v0.2.2 (/home/be/sw/slint/internal/backends/gl) default,fontconfig-dlopen,resvg,rtti,svg,tiny-skia,usvg,wayland,x11 (*)
└── i-slint-compiler v0.2.2 (/home/be/sw/slint/internal/compiler) codemap,codemap-diagnostic,cpp,display-diagnostics,fontconfig-dlopen,proc-macro2,proc_macro_span,quote,rust
    ├── slint-build v0.2.2 (/home/be/sw/slint/api/rs/build) default
    │   [build-dependencies]
    │   ├── gallery v0.2.2 (/home/be/sw/slint/examples/gallery) 
    │   ├── imagefilter v0.2.2 (/home/be/sw/slint/examples/imagefilter) 
    │   ├── memory v0.2.2 (/home/be/sw/slint/examples/memory) 
    │   ├── opengl_underlay v0.2.2 (/home/be/sw/slint/examples/opengl_underlay) 
    │   ├── plotter v0.2.2 (/home/be/sw/slint/examples/plotter) fontconfig-dlopen
    │   ├── printerdemo v0.2.2 (/home/be/sw/slint/examples/printerdemo/rust) 
    │   ├── printerdemo_mcu v0.2.2 (/home/be/sw/slint/examples/printerdemo_mcu) chrono,mcu-pico-st7789,mcu-simulator
    │   ├── printerdemo_old v0.2.2 (/home/be/sw/slint/examples/printerdemo_old/rust) 
    │   ├── slide_puzzle v0.2.2 (/home/be/sw/slint/examples/slide_puzzle) 
    │   └── todo v0.2.2 (/home/be/sw/slint/examples/todo/rust) 
    │   [dev-dependencies]
    │   └── slint v0.2.2 (/home/be/sw/slint/api/rs/slint) backend-gl-all,backend-gl-wayland,backend-gl-x11,backend-qt,compat-0-2-0,default,document-features,std (*)
    ├── slint-compiler v0.2.2 (/home/be/sw/slint/tools/compiler) 
    ├── slint-fmt v0.2.2 (/home/be/sw/slint/tools/fmt) 
    ├── slint-interpreter v0.2.2 (/home/be/sw/slint/internal/interpreter) backend-gl-all,backend-gl-wayland,backend-gl-x11,backend-qt,compat-0-2-0,default,display-diagnostics,document-features,ffi,spin_on,std (*)
    ├── slint-lsp v0.2.2 (/home/be/sw/slint/tools/lsp) backend-gl-all,backend-gl-wayland,backend-gl-x11,backend-qt,default
    ├── slint-node v0.2.2 (/home/be/sw/slint/api/node/native) 
    └── syntax_updater v0.2.2 (/home/be/sw/slint/tools/syntax_updater) 
    [dev-dependencies]
    ├── doctests v0.2.2 (/home/be/sw/slint/tests/doctests) 
    └── test-driver-cpp v0.2.2 (/home/be/sw/slint/tests/driver/cpp)

@Be-ing Be-ing marked this pull request as ready for review April 5, 2022 00:43
internal/backends/gl/build.rs Outdated Show resolved Hide resolved
@Be-ing Be-ing force-pushed the fontconfig-dlopen branch 2 times, most recently from 082e2d0 to 2642667 Compare April 5, 2022 02:23
examples/plotter/Cargo.toml Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 5, 2022

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 sudo dnf install aarch64-linux-gnu-{binutils,gcc,glibc} and add this to ~/.cargo/config.toml:

[target.aarch64-unknown-linux-gnu]
linker = "aarch64-linux-gnu-gcc"
ar = "aarch64-linux-gnu-gcc-ar"

then it just worked with

RUST_FONTCONFIG_DLOPEN=on RUST_JACK_DLOPEN=on cargo build --target aarch64-unknown-linux-gnu --no-default-features --features wayland

Copy link
Member

@ogoffart ogoffart left a 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

examples/plotter/Cargo.toml Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 5, 2022

I have the feeling that dlopen should actually be the default.

I think that would be difficult to change at this point. build.rs behaves the same way using the same RUST_FONTCONFIG_DLOPEN environment variable up and down the stack from yeslogic-fontconfig-sys, fontconfig, font-kit, i-slint-backend-gl, and i-slint-compiler.

I know that packagers don't like it, but they can play with compilation flags

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 dlopening with the environment variable because that is the default behavior.

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 6, 2022

I have the feeling that dlopen should actually be the default.

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 dlopen the default, but Slint is not written to handle the case of fontconfig not being available. Of course if we could replace the fontconfig C library with a pure Rust solution, that would be best.

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
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 30, 2022

The combination of --workspace and --all-features as the CI uses and feature-resolver = 2 in the workspace's Cargo.toml created a strange situation. The plotter example was depending on yeslogic-fontconfig-sys with the dlopen feature because of i-slint-backend-gl and i-slint-compiler's fontconfig-dlopen features. However, plotters did not activate the source-fontconfig-dlopen feature of font-kit required for font-kit to work with yeslogic-fontconfig-sys's dlopen feature:

I resolved this by adding a fontconfig-dlopen feature to plotters to activate font-kit's source-fontconfig-dlopen feature in plotters-rs/plotters#337 then exposing it as a feature in the plotter example. This way --workspace --all-features activates font-kit's source-fontconfig-dlopen feature when the fontconfig-dlopen features of i-slint-backend-gl and i-slint-compiler are enabled by --all-features.

Simpler solution: don't expose fontconfig-dlopen features in i-slint-backend-winit and i-slint-compiler. There's not much point exposing these if they aren't exposed in the user-facing slint crate anyway. The RUST_FONTCONFIG_DLOPEN=ON environment variable needs to be set to enable dlopen'ing.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 30, 2022

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\"");
Copy link
Member

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?

Copy link
Contributor Author

@Be-ing Be-ing Oct 1, 2022

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.

Copy link
Member

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.

Copy link
Contributor Author

@Be-ing Be-ing Oct 4, 2022

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!.

Copy link
Member

@tronical tronical left a 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).
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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.

Copy link
Member

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 :)

@tronical tronical merged commit 8953903 into slint-ui:master Oct 5, 2022
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.

4 participants