Created attachment 347236 [details] [review]
repo: Add imports.gi.has() to check for symbol availability

GI allows to specify the API version to import, but not a minimum
version of said API. This can lead to hard to track down bugs at
runtime, in particular when the failure doesn't trigger a warning
or exception (for example when setting a non-existent GObject
property).
Autoconf-archive provides an AX_CHECK_GIR_SYMBOLS_GJS() macro to
address this, but it's obviously only available for autoconf. In
order to have this functionality available in other contexts (at
runtime, or when using other build systems), it seems useful to
provide it in gjs itself, so add imports.gi.has() for this purpose.

While it is inspired by the autoconf macro, its implementation is
different, so the behavior is not an exact match - most notable
differences are:

 - signals and properties are supported using gtk-doc
   syntax (::signal-name and :prop-name)

 - methods and vfuncs are supported, but don't take the
   class hierarchy into account (e.g. Application.vfunc_startup()
   succeeds for Gio, but fails for Gtk)

Created attachment 347237 [details] [review]
tests: Add test cases for imports.gi.has()

I'll have to think about this some more.

It would be good to have tools to make build systems as painless as possible. On the other hand, to me it feels like those tools belong as extensions of the build system.

Apart from that, I'm not convinced about this particular implementation of it, because it encodes all the rules for translating from GIR to JS (e.g. prefixing "vfunc_", enum members casing) separately from where it is done during actual GIR lookups during code execution.

Could we instead have a JS tool that checks for the existence of the requested APIs in JS? That way, it would take inheritance into account, and you would be checking for exactly the APIs you use in your code.

Review of attachment 347237 [details] [review]:

I'd prefer this one squashed into the main patch.

Review of attachment 347236 [details] [review]:

Thanks for the patch. A few style comments, but see discussion on bug about the bigger picture :-)

::: gi/repo.cpp
@@ +194,3 @@
 
+static bool
+info_has_property (GIBaseInfo *info,

style: No space before function parens

@@ +209,3 @@
+    if (g_type_is_a(gtype, G_TYPE_INTERFACE)) {
+        GTypeInterface *iface_type =
+    if (!GI_IS_REGISTERED_TYPE_INFO(info))

style: Prefer C++ casts (static_cast<>)

(In reply to Philip Chimento from comment #3)
> It would be good to have tools to make build systems as painless as
> possible. On the other hand, to me it feels like those tools belong as
> extensions of the build system.

The meson devs disagree with that, and technically they are right that this is not a build dependency.


> Apart from that, I'm not convinced about this particular implementation of
> it, because it encodes all the rules for translating from GIR to JS (e.g.
> prefixing "vfunc_", enum members casing) separately from where it is done
> during actual GIR lookups during code execution.

Yeah. The patch started out a lot simpler, but got more involved when adding support for vfuncs/signals/properties.


> Could we instead have a JS tool that checks for the existence of the
> requested APIs in JS? 

imports.gi seemed like a logical place to hook into, which unfortunately means C. How about using imports.package.requireSymbol() instead? At least for functions/methods/enums/signals, an implementation should be straight-forward. I don't see a way to check for a GObject property from JS right now, but I guess we can leave that out initially ...

(In reply to Florian Müllner from comment #6)
> (In reply to Philip Chimento from comment #3)
> > It would be good to have tools to make build systems as painless as
> > possible. On the other hand, to me it feels like those tools belong as
> > extensions of the build system.
> 
> The meson devs disagree with that, and technically they are right that this
> is not a build dependency.

Technically correct ... the best kind of correct! But also not very helpful of them :-)

> How about using imports.package.requireSymbol() instead? At least
> for functions/methods/enums/signals, an implementation should be
> straight-forward.

I really like this idea a lot. It moves the check to runtime in a way that still makes sense.

> I don't see a way to check for a GObject property from JS
> right now, but I guess we can leave that out initially ...

Here's how:
GObject.Object.find_property.call(SomeClass, 'property-name') !== null

Created attachment 347608 [details] [review]
package: Add requireSymbol() to check for symbol availability

(In reply to Philip Chimento from comment #7)
> Here's how:
> GObject.Object.find_property.call(SomeClass, 'property-name') !== null

Witchcraft, thanks! At least I figured out the GInterface equivalent myself ...

Review of attachment 347608 [details] [review]:

Looks fine, but I think it can be made even better. The part I'm having doubts about is the API itself. To me, using ::, :, and . to distinguish signals, properties, and other members is a C-ism (or maybe a Gtk-doc-ism) and doesn't match anything else used in GJS.

How about an API like this?

    Package.requireSymbol('Gtk', '3.0', 'ShortcutsWindow');
    Package.requireSymbol('Gtk', '3.0', 'FontChooser.get_font_map');
    Package.requireSymbol('Gtk', '3.0', 'PlacesSidebar', {
        Signals: [ 'show-other-locations' ],
        Properties: [ 'populate-all' ],
    });

Come to think of it, it would be even better to have a dot denote properties too, maybe like

    Package.requireSymbol('Gtk', '3.0', 'PlacesSidebar.populate_all');

since that's what it looks like in actual GJS code. It would be easy enough, if there is no member on the JS prototype, then we check for a property on the class. Although that may make things more confusing.

Signals are still kind of the oddball here, but I don't have any better ideas right now.

PS. Do you think we should request a freeze break for this?

Florian, are you interested in picking this up again? The 1.49.1 release is in one week, and it would be nice to land it in time for that.

I was also thinking it might be nice to have a Package.checkSymbol() variant so that you could optionally take advantage of newer features but still fall back to old features, but maybe that is too much scope creep :-)

(In reply to Philip Chimento from comment #10)
> Florian, are you interested in picking this up again?

Thanks for the reminder, yes - I'll be back from vacations on Wednesday, let's have a quick chat then and I'll pick it up. In a nutshell, I like using dots for both method and properties, but I don't have a good idea for signals. (I'm not too convinced by the Signals/Properties object properties suggestion, as our custom syntax for defining GObjects strikes me as a peculiar way for testing done symbol's existence).

I reckon that checkSymbol() would be a non fatal variant that returns the test result rather than exciting on failure? That could indeed be a useful addition...

Created attachment 351487 [details] [review]
package: Add checkSymbol() to check for symbol availability

GI allows to specify the API version to import, but not a minimum
version of said API. This can lead to hard to track down bugs at
runtime, in particular when the failure doesn't trigger a warning
or exception (for example when setting a non-existent GObject
property).
To address this, add a checkSymbols() method that test whether a
library can be imported and contains a particular symbol.

Created attachment 351488 [details] [review]
package: Add requireSymbol() method

While the existing require() method provides a convenient way
to check for hard dependencies, it only tests whether a particular
typelib can be imported. The newly added checkSymbol() method on
the other hand allows for finer-grained tests, but leaves it to
the caller to check the return value. Fill that gap by providing
a requireSymbol() method that wraps checkSymbol() to error out
when the dependency is not satisfied.

I've still no good idea for signal syntax, so I left them out for now.

Review of attachment 351487 [details] [review]:

Thanks! Just some JS style comments.

It would be nice to have a test that checks that you can find a method on an interface as well. There aren't any good ones in Regress, but you could check for the existence of GIMarshallingTests.Interface.test_int8_in().

::: modules/package.js
@@ +281,3 @@
+
+    let [klass, sym] = symbol.split('.');
+ */

nitpick: Prefer ===

@@ +288,3 @@
+        return false;
+
+

nitpick: Prefer !==

@@ +301,3 @@
+    }
+
+        return false;

nitpick: Prefer !==

Review of attachment 351488 [details] [review]:

+1, can be committed when the other one is ready, modulo the semicolon. Thanks!

::: modules/package.js
@@ +246,3 @@
+    if (!checkSymbol(lib, version, symbol)) {
+        if (symbol)
+ * dependency cannot be satisfied.

optional: if you like, use the new template literal syntax, with backticks :-)

    printerr(`Unsatisfied dependency: No ${symbol} in ${lib}`);
    printerr(`Unsatisfied dependency: ${lib}`);

@@ +250,3 @@
+            printerr('Unsatisfied dependency: ' + lib);
+        System.exit(1);
+function requireSymbol(lib, version, symbol) {

nitpick: unnecessary semicolon

Created attachment 351518 [details] [review]
package: Add checkSymbol() to check for symbol availability

The splinter output was a bit messed up, so I hope I fixed all the right comparisons. Also added tests for interfaces/interface methods as suggested.

Created attachment 351519 [details] [review]
package: Add requireSymbol() method

Fix nitpick and use backtick syntax.

Attachment 351518 [details] pushed as 588f400 - package: Add checkSymbol() to check for symbol availability
Attachment 351519 [details] pushed as ca34972 - package: Add requireSymbol() method


Thanks! If you come up with something for signals we can add it in the future.