-
Notifications
You must be signed in to change notification settings - Fork 567
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
Make GTK dependency optional. #1241
Conversation
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 nice improvement for x11 users, thanks for digging in. I have a few little concerns inline, happy to merge this when we've figured those things out.
druid-shell/Cargo.toml
Outdated
@@ -14,7 +14,9 @@ rustdoc-args = ["--cfg", "docsrs"] | |||
default-target = "x86_64-pc-windows-msvc" | |||
|
|||
[features] | |||
x11 = ["x11rb", "nix", "cairo-sys-rs"] | |||
x11 = ["x11rb", "nix", "cairo-sys-rs", "cairo-rs"] | |||
GTK = ["cairo-rs", "gio", "gdk", "gdk-sys", "glib", "glib-sys", "gtk-sys", "gtk"] |
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 just call this gtk
? It's okay if this collides with the gtk
crate I believe.
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 also prefer just calling it gtk
, but Cargo does not agree with us :(
(It will error about two features with the same name)
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.
what about platform-gtk
?
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.
Okay, what I propose:
in druid-shell/Cargo.toml
, rename the actual gtk crate to gtk-rs
, and then call the feature gtk
:
[features]
x11 = ["x11rb", "nix", "cairo-sys-rs", "cairo-rs"]
gtk = ["cairo-rs", "gio", "gdk", "gdk-sys", "glib", "glib-sys", "gtk-sys", "gtk-rs"]
default = ["gtk"]
[target.'cfg(target_os="linux")'.dependencies]
# other deps here
gtk-rs = { version = "0.9.2", features = ["v3_22"], package = "gtk", optional = true }
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 that did the job 👍
druid-shell/Cargo.toml
Outdated
gtk-sys = "0.10.0" | ||
gtk = { version = "0.9.2", features = ["v3_22"] } | ||
# TODO(x11/dependencies): only use feature "xcb" if using X11 | ||
cairo-rs = { version = "0.9.1", default_features = false, features = ["xcb"], optional = true } |
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.
we currently allow cairo to be used, opt-in, on macOS. I'm not sure we should, but we do; I think this is why these deps are in two places.
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.
Huh I did not know that. I'll move Cairo back to the "normal" dependencies then.
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 proved to be a bit tricky, but it should work now.
cbbe96e
to
54ce883
Compare
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 looks good. I think we should actually remove the cairo-on-mac stuff, it isn't even wired up to druid anymore (it only works for druid-shell) and it just isn't worth the ongoing maintenance cost; I'll open an issue for that though, it's definitely a separate patch.
Thanks!
This adds a new feature
GTK
that gates all its related dependencies, which allows building the X11 backend without GTK.GTK
is a default feature, which means this won't change anything for existing usages, but withone can use Druid with X11 and no GTK bits involved 🎉
This seems like a simple working solution, if it compiles on Windows and Mac.
Also, I had to give the feature an upper case name, otherwise it would have conflicted with the
gtk
feature...Maybe a different name would be less ambiguous, but I could not come up with one.