Closed Bug 1614266 Opened 3 years ago Closed 2 years ago

[Wayland] Use GDK guard macros to detect wayland/X environment instead of gdk_x11_display_get_type

Categories

(Core :: Widget: Gtk, enhancement, P3)

Product:
Core
Component:
Widget: Gtk
Version:
73 Branch
Platform:
x86_64
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1696845

People

(Reporter: amit.prakash.ambasta, Unassigned)

References

(Blocks 1 open bug)

Details

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Currently, firefox uses gdk_x11_display_get_type to check if firefox is running in a wayland environment or an X environment. This requires runtime dependency on gtk-x11.

Steps to reproduce:
Build mesa, cairo, gtk+, cairomm and gtkmm without X support.
Launch firefox-bin

Actual results:

/opt/firefox/firefox: symbol lookup error: /opt/firefox/libxul.so: undefined symbol: gdk_x11_display_get_type

Expected results:

Firefox should be able to launch w/o X support in gtk/gdk

Type: defect → enhancement

gdk exposes GDK_WINDOWING_WAYLAND and GDK_WINDOWING_X11 as guard macros which can be used to detect which variation of display we're on. https://developer.gnome.org/gdk3/stable/gdk3-General.html#GDK-WINDOWING-X11:CAPS

Summary: Use GDK guard macros to detect wayland/X environment instead of gdk_x11_display_get_type → [Wayland] Use GDK guard macros to detect wayland/X environment instead of gdk_x11_display_get_type
Component: Untriaged → Widget: Gtk
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64

Okay, I expect we can create some custom firefox macro with the guard.

Blocks: wayland
Priority: -- → P3

Using GDK_IS_WAYLAND_DISPLAY(display) instead of !GDK_IS_X11_DISPLAY(display) might solve this issue since GDK_IS_X11_DISPLAY(display) expands to G_TYPE_CHECK_INSTANCE_TYPE(display, gdk_x11_display_get_type()).

(In reply to user142 from comment #4)

Using GDK_IS_WAYLAND_DISPLAY(display) instead of !GDK_IS_X11_DISPLAY(display) might solve this issue since GDK_IS_X11_DISPLAY(display) expands to G_TYPE_CHECK_INSTANCE_TYPE(display, gdk_x11_display_get_type()).

GDK_IS_WAYLAND_DISPLAY() breaks mozilla internal build systems....we used that before. Feel free to create such patch I can try it again. AFAIK GDK_IS_WAYLAND_DISPLAY() needs gtk wayland headers to build and mozilla supports to build firefox without wayland headers.

To be clear here - feel free to provide a patch for it and I can it review and test on mozilla build systems.

(In reply to Martin Stránský [:stransky] from comment #6)

To be clear here - feel free to provide a patch for it and I can it review and test on mozilla build systems.

Is there a reason GTK's method wouldn't work?

#ifdef GDK_WINDOWING_WAYLAND
  if (GDK_IS_WAYLAND_DISPLAY (display))
    {
     stuff();
    }
#endif

Sorry if that isn't what you're looking for, I don't know a lot about this, just want to run Firefox on my Wayland-only system with less dependencies. :)

Flags: needinfo?(stransky)

GDK_WINDOWING_WAYLAND macro needs wayland include files which are not present on X11 only systems.

Flags: needinfo?(stransky)

I was under the impression GDK_WINDOWING_WAYLAND, unlike GDK_IS_WAYLAND_DISPLAY (), was provided by GTK and is functional without Wayland include files. After all, GDK in their own code use it to guard Wayland only functionality without breaking anything on non-Wayland systems: https://github.com/GNOME/gtk/blob/b353221185c53ea7a96b9e27ba45bfe90e1cdf87/gtk/inspector/general.c#L323.

(In reply to shpilenok2001 from comment #9)

I was under the impression GDK_WINDOWING_WAYLAND, unlike GDK_IS_WAYLAND_DISPLAY (), was provided by GTK and is functional without Wayland include files. After all, GDK in their own code use it to guard Wayland only functionality without breaking anything on non-Wayland systems: https://github.com/GNOME/gtk/blob/b353221185c53ea7a96b9e27ba45bfe90e1cdf87/gtk/inspector/general.c#L323.

Ah sure, you're right.

Sweet, seems like it should be a pretty easy fix then. :)

Do you mind to create a patch for it?
Thanks.

Flags: needinfo?(shpilenok2001)

@Martin

I am not sure how. Currently, the only place where I see gdk_x11_display_get_type is in mozgtk.c defined as a STUB, I am not sure how this exapands to a gtk function call.

Look for GDK_IS_X11_DISPLAY(). This is a macro which expands to gdk_x11_display_get_type() call.

I'll be free in a week and am planning to try to make a patch then.

Flags: needinfo?(shpilenok2001)

(In reply to Martin Stránský [:stransky] from comment #5)

(In reply to user142 from comment #4)

Using GDK_IS_WAYLAND_DISPLAY(display) instead of !GDK_IS_X11_DISPLAY(display) might solve this issue since GDK_IS_X11_DISPLAY(display) expands to G_TYPE_CHECK_INSTANCE_TYPE(display, gdk_x11_display_get_type()).

GDK_IS_WAYLAND_DISPLAY() breaks mozilla internal build systems....we used that before. Feel free to create such patch I can try it again. AFAIK GDK_IS_WAYLAND_DISPLAY() needs gtk wayland headers to build and mozilla supports to build firefox without wayland headers.

I am making a patch today and am trying to understand what you mean here.

Do you mean to say that Mozilla internal build systems must be able to build Firefox with Wayland support enabled, even without Wayland headers?
Because, if not, aren't all areas that check for a Wayland display already guarded by MOZ_WAYLAND, and so it would be fine to use GDK_IS_WAYLAND_DISPLAY because MOZ_WAYLAND requires Wayland headers?

Or are there places where Wayland support is needed that are not guarded by MOZ_WAYLAND? I suppose in that case one would have to construct some custom MOZ_IS_WAYLAND macro, like in Comment #7? If so, where should I put it?

Flags: needinfo?(stransky)

Or I suppose GDK_IS_WAYLAND_DISPLAY and MOZ_WAYLAND should be identical, correct?

No, MOZ_WAYLAND is a build switch. Builds built with MOZ_WAYLAND can still run under X.

GDK_WINDOWING_WAYLAND is a macro that is available regardless of if wayland headers are available.

In fact, gtk exposes couple of these macros, GDK_WINDOWING_X11, GDK_WINDOWING_WIN32, GDK_WINDOWING_QUARTZ, GDK_WINDOWING_WAYLAND

Ideally, all wayland/X specific code should be behind these guard macros. Additionally, GDK_IS_*_DISPLAY should be invoked only if respective macros are available, which would allow for building wayland support w/o X and vice versa.

MOZ_WAYLAND makes gdkwayland mandatory, therefore the availability of GDK_IS_WAYLAND_DISPLAY, therefore also implies GDK_WINDOWING_WAYLAND. In other words GDK_IS_WAYLAND_DISPLAY will always be available if MOZ_WAYLAND is set.

IMO, GDK_IS_DISPLAY should not be called negatively (!GDK). But GDK_IS_WAYLAND_DISPLAY should be guarded by MOZ_WAYLAND or GDK_WINDOWING_WAYLAND ifdefs to avoid breakage on non-wayland system. MOZ_WAYLAND and MOZ_X11 are widely used throughout the source code, therefore I think they should be used to guard GDK_IS_DISPLAY instead of GDK_WINDOWING.

From a quick grep, usage of !GDK_IS_X11_DISPLAY(display) is already very often guarded by MOZ_WAYLAND. Changing those should be trivial, for the others, ifdef MOZ_WAYLAND should probably be added.

I'm not a firefox developer, do you agree with my analysis?

I agree that GDK_IS_WAYLAND_DISPLAY is better than !GDK_IS_X11_DISPLAY (and the other way around), especially when explicitly guarded by MOZ_WAYLAND. And also that eventually all X11/Wayland specific code should be wrapped by such guards (MOZ_WAYLAND/MOZ_X11).

I personally try to use exactly that pattern in my patches already (e.g. https://phabricator.services.mozilla.com/D76417), but so far there was no explicit consent as far as I know.

I understood you that you meant a pattern like this, right?

#ifdef MOZ_WAYLAND
if (GDK_IS_WAYLAND_DISPLAY(display)) {
...
}
#endif
#ifdef MOZ_X11
if (GDK_IS_X11_DISPLAY(display)) {
...
}
#endif

(In reply to Robert Mader [:rmader] from comment #22)

I agree that GDK_IS_WAYLAND_DISPLAY is better than !GDK_IS_X11_DISPLAY (and the other way around), especially when explicitly guarded by MOZ_WAYLAND. And also that eventually all X11/Wayland specific code should be wrapped by such guards (MOZ_WAYLAND/MOZ_X11).

I personally try to use exactly that pattern in my patches already (e.g. https://phabricator.services.mozilla.com/D76417), but so far there was no explicit consent as far as I know.

I understood you that you meant a pattern like this, right?

#ifdef MOZ_WAYLAND
if (GDK_IS_WAYLAND_DISPLAY(display)) {
...
}
#endif
#ifdef MOZ_X11
if (GDK_IS_X11_DISPLAY(display)) {
...
}
#endif

Yes, exactly

I'm working on bug #1661450 to build firefox on wayland without X11 and looked at this as it may be similar or a subset of bug #1661450. (I'm doing some tests right now and soon publish some patches there)

I'm not sure what you want to accomplish here:

  • be able to compile on wayland without X support? Then this a duplicate of bug #1661450, isn't it?
  • or be able to run a firefox compiled for both X11 and Wayland on a system without X support? then this probably cannot be achieved with GDK_WINDOWING_WAYLAND, MOZ_WAYLAND macros because they are compile time macros; the runtime macro GDK_IS_WAYLAND_DISPLAY requires wayland and GDK_IS_X11_DISPLAY requires X. So there is probably not an easy solution. You need a runtime check that does neither rely on X11 nor on Wayland. I not sure how you could do this.
Flags: needinfo?(stransky)

Should be fixed by bug 1696845

Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.