Created attachment 295866 [details] [review]
girepository: Document semantics of dependencies and includes better

Make it clear that both the dependencies field in the typelib, and the
include elements in the GIR AST, are for immediate dependencies, not
transitive ones.

Created attachment 295867 [details] [review]
girepository: Fix NULL return from g_irepository_get_dependencies()

If a typelib had no dependencies, g_irepository_get_dependencies() would
return NULL, rather than an empty NULL-terminated vector.

Created attachment 295868 [details] [review]
girepository: Add g_irepository_get_immediate_dependencies()

g_irepository_get_dependencies() is supposed to return the transitive
closure of all dependencies of the given namespace. However, it just
loads the dependencies field from the typelib, which is supposed to only
list immediate dependencies.

Introduce a new g_irepository_get_immediate_dependencies() which does
this, and rewrite g_irepository_get_dependencies() to build the
transitive closure of all its namespace dependencies. This does not
require loading any new typelibs, as the transitive closure of
dependencies should already have been loaded by g_irepository_require()
or g_irepository_load_typelib().

Thanks for the updates.  g_irepository_get_dependencies appears to be giving the full transitive list now - I tested a few examples.  Also, I am in favour of it never returning null.  I didn't check g_irepository_get_immediate_dependencies though (I was testing via bindings to girepository).

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

Review of attachment 295867 [details] [review]:

I'm not totally sure about changing a public API in a subtle way like this, but I guess any callers should be prepared to handle it.

Review of attachment 295868 [details] [review]:

Can you provide a rationale behind changing the existing API to become transitive instead of adding a new g_irepository_get_transitive_dependencies() or so?

I don't view this as a change in API: we used to get the transitive dependencies because the include list in the GIR files contained the transitive dependencies.  Also, the doc for g_irepository_get_dependencies has always said the list was transitive.  I see this function as becoming broken when the GIR files were changed to include only the immediate dependencies.

Also, would the name
  g_irepository_get_includes 
be preferable to
  g_irepository_get_immediate_dependencies
?

(In reply to Colin Walters from comment #6)
> Review of attachment 295867 [details] [review] [review]:
> 
> I'm not totally sure about changing a public API in a subtle way like this,
> but I guess any callers should be prepared to handle it.

As Phil Clayton says, g_irepository_get_dependencies() is already documented as returning the transitive closure of dependencies.

(In reply to Phil Clayton from comment #9)
> Also, would the name
>   g_irepository_get_includes 
> be preferable to
>   g_irepository_get_immediate_dependencies
> ?

Maybe, though I would be wary of the two names being too similar — there’s nothing to differentiate ‘includes’ from ‘dependencies’ unless you know the element names in the GIR format. I guess I don’t mind about the naming either way.

Comment on attachment 295867 [details] [review]
girepository: Fix NULL return from g_irepository_get_dependencies()

Attachment 295867 [details] pushed as c74a18a - girepository: Fix NULL return from g_irepository_get_dependencies()

Review of attachment 295866 [details] [review]:

Right.

(In reply to Philip Withnall from comment #10)
> (In reply to Colin Walters from comment #6)
> > Review of attachment 295867 [details] [review] [review] [review]:
> > 
> > I'm not totally sure about changing a public API in a subtle way like this,
> > but I guess any callers should be prepared to handle it.
> 
> As Phil Clayton says, g_irepository_get_dependencies() is already documented
> as returning the transitive closure of dependencies.

I think in general when the code disagrees with the documentation, a conservative principle is to avoid application breakage by changing the docs to be correct, and adding a new API.

That said, I am not opposed to changing the code to match the docs (the intent is clear!), and if you feel strongly, please go ahead and commit the patch.

I suspect though this will at least change the behavior of existing apps.  For example, girwriter.c.

Attachment 295866 [details] pushed as 6ff0b08 - girepository: Document semantics of dependencies and includes better
Attachment 295868 [details] pushed as 8480bf5 - girepository: Add g_irepository_get_immediate_dependencies()