This is a bug with how introspection works.

introspection calls API from libraries it introspects (notably
it calls class initializers by way of g_type_class_ref (LIBRARY_GTYPE)),
but it fails to initialize those libraries where some libraries need
to initialized.

This is not encouraging because for libraries such as GTK+,
initializing the library normally requires a DISPLAY.

In the case of Glade particularly, this started failing because
one of the dynamic enumeration types we use needs to call
gtk_stock_list_ids(), and gtk_stock_list_ids() stopped working
without proper initialization of GTK+ (and without a display).

Again, the problem points back to how gobject-introspection
needs to call API from the libraries it introspects, but fails
to require initialization.

(In reply to comment #1)
> This is a bug with how introspection works.

Introspection works the same way gtk-doc has always worked.  Yes, it's not ideal, but we have had things building without DISPLAY set (i.e. without calling gtk_init()) for years.  I don't see a need to change that now, it's often not very difficult to change code run in class_init() to code run inside a GOnce in _init() or equivalent.

(In reply to comment #2)
> (In reply to comment #1)
> > This is a bug with how introspection works.
> 
> Introspection works the same way gtk-doc has always worked.  Yes, it's not
> ideal, but we have had things building without DISPLAY set (i.e. without
> calling gtk_init()) for years.  I don't see a need to change that now, it's
> often not very difficult to change code run in class_init() to code run inside
> a GOnce in _init() or equivalent.

Colin, sure, we've been doing it forever, and exactly, it's not ideal.

It's really not uncommon for class initializers to call API and set
things up for the duration of a class's life cycle, if that requires
a DISPLAY connection, then it requires a DISPLAY connection.

Any patches that I've seen so far addressing the issue with GTK+ 3.9.x,
have been workarounds to do something different at class_init() time.

There is no reason to claim these classes were doing something wrong
before this bug popped up, so we are obviously patching up the platform
for a problem which is deeper, a problem I outlined in comment 1.

FWIW, in Glade, the only reasonable way of hacking around this bug,
without refactoring hundreds of lines of code, is to do something
completely broken (i.e., an empty ENUM ?) when generating the stock
GEnum when there is no DISPLAY available ... just for the sake of
working around this deeper issue.

(In reply to comment #3)
>
> FWIW, in Glade, the only reasonable way of hacking around this bug,
> without refactoring hundreds of lines of code, is to do something
> completely broken (i.e., an empty ENUM ?) when generating the stock
> GEnum when there is no DISPLAY available ... just for the sake of
> working around this deeper issue.

Would my suggestion of GOnce in _init work in this case?

(In reply to comment #4)
> (In reply to comment #3)
> >
> > FWIW, in Glade, the only reasonable way of hacking around this bug,
> > without refactoring hundreds of lines of code, is to do something
> > completely broken (i.e., an empty ENUM ?) when generating the stock
> > GEnum when there is no DISPLAY available ... just for the sake of
> > working around this deeper issue.
> 
> Would my suggestion of GOnce in _init work in this case?

Not in this case no.

We use gtk_stock_list_ids() to actually generate the enum values
array which we feed to g_enum_register_static() (so that code
runs directly from our _get_type() function for that enum type).

It could be done differently, but as I mentioned, that would mean
refactoring the code to not use an enum type for the stock id selector
editors (not exactly an easy quick fix).

The quick and dirty fix to work around this issue would be to just
not register the stock list when DISPLAY is NULL (which would result
in an empty enumeration in the GIR output, which is not really a big
deal as I don't see that enum as important to add to the GIR).

(In reply to comment #5)

> The quick and dirty fix to work around this issue would be to just
> not register the stock list when DISPLAY is NULL 

But remember this is the *normal case*.  Very broadly speaking, 98.3% of users get their glade binaries from packages like deb/rpm.  99.9% of those packages are built using pbuilder/mock which will not have DISPLAY set.  So this code cannot have its intended effect.

(In reply to comment #6)
> (In reply to comment #5)
> 
> > The quick and dirty fix to work around this issue would be to just
> > not register the stock list when DISPLAY is NULL 
> 
> But remember this is the *normal case*.  Very broadly speaking, 98.3% of users
> get their glade binaries from packages like deb/rpm.  99.9% of those packages
> are built using pbuilder/mock which will not have DISPLAY set.  So this code
> cannot have its intended effect.

Sure it can.

Glade does not use the GIR information to introspect what stock ids are
available, instead Glade uses the GType of an enum, the enum itself
has it's _get_type() called when Glade actually runs, and while initializing
the type on a system where Glade runs, it will be populated with values
from gtk_stock_list_ids() as usual.

Only the GIR will not contain an enum with the stock ids listed as
enum values (which as I mentioned, is not important anyway).

It's a plausible workaround afaics.

My opinion: glade should really introspect these values at runtime. Thats the only time we know that a DISPLAY will be available.

(In reply to comment #7)

> Sure it can.

You're correct; sorry, I went off the rails briefly.

> Only the GIR will not contain an enum with the stock ids listed as
> enum values (which as I mentioned, is not important anyway).
> 
> It's a plausible workaround afaics.

Yeah, I guess.  But it's still quite ugly to have the situation where the GType for GladeStock can only be accessed after gtk_init() has been invoked.

There are other approaches; Glade could dump the stock ids from gtk+ at *build* time; then it would need to be rebuilt after new stock items are added to gtk+, but it'd be more reliable.

Why does glade have an enumeration mirroring the stock ids anyways as opposed to just having callers use the strings?

(In reply to comment #8)
> My opinion: glade should really introspect these values at runtime. Thats the
> only time we know that a DISPLAY will be available.

It does introspect them at runtime, and creates an enumeration based on
that, at runtime... however GIR introspection also happens at run time.

(In reply to comment #9)
> (In reply to comment #7)
[...]
> 
> Why does glade have an enumeration mirroring the stock ids anyways as opposed
> to just having callers use the strings?

As I mentioned, it could be done differently, by rewriting a bunch of
code (hence why I guess the dirty workaround is a better short-term
solution).

Why did we originally do it this way ? Because we generate combo box
editors for GParamEnum properties, so to edit stock button strings
we internally pretend that they are enum properties and 'poof' up comes
a combo box (of course, things have evolved a bit since then, but we
still carry the GValue internally as an enumeration, not a string).

Fun fact: we also let catalogs add to GTK+'s stock list, this happens
before the stock enum type is dynamically generated (and that's how we
supported the extra stock items which libgnomeui provided).

(In reply to comment #9)
> (In reply to comment #7)
> 
> > Sure it can.
> 
> You're correct; sorry, I went off the rails briefly.
> 
> > Only the GIR will not contain an enum with the stock ids listed as
> > enum values (which as I mentioned, is not important anyway).
> > 
> > It's a plausible workaround afaics.
> 
> Yeah, I guess.  But it's still quite ugly to have the situation where the GType
> for GladeStock can only be accessed after gtk_init() has been invoked.

And I definitely disagree with this, while yes... Glade could be refactored
to work differently wrt editing stock strings... this exact statement is
IMO incorrect.

Glade depends on GTK+, why does it make any sense that any Glade code
should be callable before gtk_init() ? That in itself is not ugly (there
may be ugly things in Glade, but that in itself is not one of them).

This is actually, the whole root of this debate IMO, of course I could be
just implementing the workaround (which I'll probably end up doing anyway),
the whole interesting part of this discussion is that introspection somehow
thinks it's OK to access code in libraries without initializing those libraries.

Sure, maybe this is something that we should end up living with ?

Maybe we should state that type initializers and class initializers
somehow don't count as API ? and document/enforce that as a rule ?

Maybe we should document/enforce that libraries should have ways to
initialize themselves in environments like build servers, where some
D-Bus services are not available, where there might not be a DISPLAY ?

Whichever it is, we should look at it.

Created attachment 244597 [details] [review]
Avoid calling gtk_stock_list_ids() when gdk_display_get_default() is NULL

Can someone test this patch and let me know if it fixes the GIR build on
build machines ?

It looks like it will do the job, while building GIRs on my machine
I get the following for the GladeStock enum type:

<enumeration name="Stock"
             glib:type-name="GladeStock"
             glib:get-type="glade_standard_stock_get_type"
             c:type="GladeStock">
  <member name="Dummy" value="0" c:identifier="dummy" glib:nick="Dummy">
  </member>
  ...
</enumeration>

(In reply to comment #12)
> Created an attachment (id=244597) [details] [review]
> Avoid calling gtk_stock_list_ids() when gdk_display_get_default() is NULL
> 
> Can someone test this patch and let me know if it fixes the GIR build on
> build machines ?

You'd likely be able to reproduce with jhbuild using something as simple as:

env -u DISPLAY make

(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=244597) [details] [review] [details] [review]
> > Avoid calling gtk_stock_list_ids() when gdk_display_get_default() is NULL
> > 
> > Can someone test this patch and let me know if it fixes the GIR build on
> > build machines ?
> 
> You'd likely be able to reproduce with jhbuild using something as simple as:
> 
> env -u DISPLAY make

Unfortunately not.

Without the above patch, if I do 'unset DISPLAY' and build Glade,
I don't get any error :-/

And I do have recent GTK+...

Comment on attachment 244597 [details] [review]
Avoid calling gtk_stock_list_ids() when gdk_display_get_default() is NULL

This patch apparently fixes the build error, committing.

Err, actually I thought I had a confirmation from Colin on IRC, but I may well
have misread/misunderstood it... I'm quite sure this workaround fixes the
issue at least for gtk_stock_list_ids(), so I'm closing this bug.

Please feel free to reopen this bug if you still have issues building Glade.