After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 621570 - Replacing gtk-doc, warnings framework
Replacing gtk-doc, warnings framework
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2010-06-14 17:09 UTC by Colin Walters
Modified: 2015-02-07 16:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Support scriptable=no attribute (78.27 KB, patch)
2010-06-14 17:10 UTC, Colin Walters
needs-work Details | Review
Add some more Gio annotations (881 bytes, patch)
2010-06-15 17:34 UTC, Colin Walters
committed Details | Review
[giscanner] Apply annotations from invoker to vfunc (4.14 KB, patch)
2010-06-15 17:35 UTC, Colin Walters
committed Details | Review
Support introspectable=no attribute, add warnings framework (90.78 KB, patch)
2010-06-15 17:35 UTC, Colin Walters
committed Details | Review
Support introspectable=no attribute, add warnings framework (93.35 KB, patch)
2010-06-17 14:18 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2010-06-14 17:09:57 UTC
This work allows us to move closer to replacing gtk-doc, among
other things.  We add a generic attribute "scriptable", and inside
the typelib compiler if we see "scriptable=no", we don't put it
in the typelib.  This replaces the hackish pre-filter for varargs
with a much more generic mechanism.

The varargs is now handled in the scanner, and we emit scriptable=no
for them.

There's a lot of reindentation in girparser.c to avoid excessive
nesting; when looking at the diff, I suggest ignoring whitespace
changes.
Comment 1 Colin Walters 2010-06-14 17:10:00 UTC
Created attachment 163612 [details] [review]
Support scriptable=no attribute
Comment 2 Dan Winship 2010-06-14 17:18:23 UTC
I don't like the name "scriptable". The typelib is used by non-"scripting" languages too.

Yes, "introspectable=no" sucks, and no I don't have a better suggestion
Comment 3 Johan (not receiving bugmail) Dahlin 2010-06-14 17:39:04 UTC
Review of attachment 163612 [details] [review]:

Didn't review most of girparser.c which I think should be moved to a separate patch.

::: giscanner/annotationparser.py
@@ +869,3 @@
     def _parse_skip(self, node, block):
         if block is not None:
             if OPT_SKIP in block.options:

Should we update the annotation option as well?

::: giscanner/girwriter.py
@@ +134,3 @@
             self.write_tag('attribute', [('name', key), ('value', value)])
 
+    def _append_node_generic(self, node, attrs):

Just _append_node() ?

@@ +136,3 @@
+    def _append_node_generic(self, node, attrs):
+        if not node.scriptable:
+            attrs.append(('scriptable', 'no'))

We use 1/0 pretty consistently here.

::: giscanner/glibtransformer.py
@@ +188,3 @@
             self._resolve_types(nodes)
 
+        self._validate(nodes)

Needs to be disabled by default until warnings/error reporting system is in place.

@@ +848,3 @@
+    # Node walking
+
+    def _walk(self, node, callback, chain):

Getting pretty tired of seeing all these node walking routines. We need to clean them up so we don't traverse the tree 5-6 times or so like we do today.

@@ +1113,3 @@
+            for vfunc in node.virtual_methods:
+                if not vfunc.invoker:
+                    print ("warning: Interface %r virtual function %r " + \

No need for \

@@ +1142,3 @@
     def _validate(self, nodes):
         for (name, node) in nodes:
+            self._walk(node, self._validate_node, [])

What about an api like this;
self._walk(node, callback, [list_of_types_that_matches]), eg:

def walk_type_container(node, stack):
   if type container
     do _scriptable_analysis
     do _scriptable_pass2
   if glib interface
     do _interface_vfunc_check

self._walk(node, walk_type_container, [TypeContainer, GLibInterface])

Which avoids traversing the tree twice.
Comment 4 Johan (not receiving bugmail) Dahlin 2010-06-14 17:47:07 UTC
I agree with Dan that 'scriptable' is not a very good name. Couple of naming suggestions;
  * exported
  * public
  * published
  * supported
  * visible

Possibly with with a "api" prefix/suffix.
Comment 5 Colin Walters 2010-06-14 18:51:30 UTC
(In reply to comment #4)
> I agree with Dan that 'scriptable' is not a very good name. Couple of naming
> suggestions;
>   * exported
>   * public
>   * published
>   * supported
>   * visible

The problem is that for e.g. functions which take varargs, unannotated GList * returns, they're *all* of those things for people using from C directly.

I'll go with "introspectable" I guess if you guys feel strongly.  It's less obvious though than "scriptable" - which is by far the most important use of introspection data, even though I agree it's not the only one.
Comment 6 Colin Walters 2010-06-14 18:58:21 UTC
(In reply to comment #2)
> I don't like the name "scriptable". The typelib is used by non-"scripting"
> languages too.

If this is the precise objection - while I know the Java bindings I was prototyping worked that way, it makes more sense if one is statically generating code to work from the .gir, I think.
Comment 7 Dan Winship 2010-06-14 19:25:35 UTC
Well, when I saw the bug summary, my first thought was that "scriptable=no" meant either "only for interactive/command-line/REPL use" or "not to be used when implementing dbus actions for other apps to invoke a la AppleScript". Which are both nonsensical things to annotate, but I couldn't think of anything else "scriptable" could refer to. I eventually figured it out after reading the description of the behavior, but if I had been first exposed to it by coming across the annotation in some source code, I would never have correctly guessed what it did.
Comment 8 Colin Walters 2010-06-14 19:45:18 UTC
(In reply to comment #7)
> Well, when I saw the bug summary, my first thought was that "scriptable=no"
> meant either "only for interactive/command-line/REPL use" or "not to be used
> when implementing dbus actions for other apps to invoke a la AppleScript".
> Which are both nonsensical things to annotate, but I couldn't think of anything
> else "scriptable" could refer to. I eventually figured it out after reading the
> description of the behavior, but if I had been first exposed to it by coming
> across the annotation in some source code, I would never have correctly guessed
> what it did.

Ah, well the source code annotation is actually (skip).  We could use that too in the GIR I guess.
Comment 9 Colin Walters 2010-06-15 17:34:54 UTC
Created attachment 163703 [details] [review]
Add some more Gio annotations
Comment 10 Colin Walters 2010-06-15 17:35:00 UTC
Created attachment 163704 [details] [review]
[giscanner] Apply annotations from invoker to vfunc

We typically expect people to annotate e.g. GList for virtuals
on the invoker, not on the virtual slot, since the invoker feels
like the public API.
Comment 11 Colin Walters 2010-06-15 17:35:07 UTC
Created attachment 163705 [details] [review]
Support introspectable=no attribute, add warnings framework

This work allows us to move closer to replacing gtk-doc, among other
things.  We add a generic attribute "introspectable", and inside the
typelib compiler if we see "introspectable=no", we don't put it in the
typelib.  This replaces the hackish pre-filter for varargs with a much
more generic mechanism.

The varargs is now handled in the scanner, and we emit
introspectable=no for them.

Add generic metadata to Node with references to file/line/column,
which currently comes from symbols.

Add scanner options --Wintrospectable and --Wfatal.
Comment 12 Colin Walters 2010-06-15 17:39:36 UTC
(In reply to comment #11)
> Created an attachment (id=163705) [details] [review]
> Support introspectable=no attribute, add warnings framework

This is now squashed with my warnings patch, because while they are sort of conceptually independent in practice they touch too many related areas.

The warnings patch is fairly small right now - but we need to add many more warnings, such as missing (transfer) on return values.

You can see examples of the warnings when scanning Gio.
Comment 13 Johan (not receiving bugmail) Dahlin 2010-06-15 20:52:37 UTC
Review of attachment 163703 [details] [review]:

Yes
Comment 14 Johan (not receiving bugmail) Dahlin 2010-06-15 20:53:15 UTC
Review of attachment 163704 [details] [review]:

Looks good.
Comment 15 Johan (not receiving bugmail) Dahlin 2010-06-16 13:02:13 UTC
Review of attachment 163705 [details] [review]:

::: giscanner/glibtransformer.py
@@ +161,3 @@
                 self._resolve_node(node)
             except KeyError, e:
+                print "WARNING: DELETING node %s: %s" % (node.name, e)

Remove this, or make it into warning

::: giscanner/scannermain.py
@@ +88,3 @@
                       action="append", dest="packages_export", default=[],
                       help="Associated pkg-config packages for this library")
+    parser.add_option('', "--Wintrospection",

Do something like this instead, do be able to get -W and multiple options;

parser.add_option('-W', '', action="append", dest="warnings", default=[],
                  "pass a warning")

If you pass in -Wall -Wsliff-sloff options.warnings will then contain ['all', 'sliff-sloff'].

Everything in that list should be verified, and a warning should be printed if there's an unknown name instead of being fatal, eg: Warning: Unknown option -Wfoobar.

Remember that all warnings needs to be documented and added to the manual page.

::: giscanner/transformer.py
@@ +125,3 @@
     # Private
 
+    def _log_node_warning(self, node, string, context=None):

This should be moved out of transformer.py and be a public function

giscanner/log:warn(node, string, context) or so.

Can probably be more flexible, to allow the callsites of this function to do most of the string formatting.
Comment 16 Colin Walters 2010-06-17 01:08:32 UTC
(In reply to comment #15)
> Review of attachment 163705 [details] [review]:
> 
> ::: giscanner/glibtransformer.py
> @@ +161,3 @@
>                  self._resolve_node(node)
>              except KeyError, e:
> +                print "WARNING: DELETING node %s: %s" % (node.name, e)
> 
> Remove this, or make it into warning

Done.

> 
> ::: giscanner/scannermain.py
> @@ +88,3 @@
>                        action="append", dest="packages_export", default=[],
>                        help="Associated pkg-config packages for this library")
> +    parser.add_option('', "--Wintrospection",
> 
> Do something like this instead, do be able to get -W and multiple options;
> 
> parser.add_option('-W', '', action="append", dest="warnings", default=[],
>                   "pass a warning")
> 
> If you pass in -Wall -Wsliff-sloff options.warnings will then contain ['all',
> 'sliff-sloff'].

But this is the thing - I don't think there's really a useful separation of warnings for us.  -Wlist-returns?  -Wbare-pointers?  Doesn't seem too interesting.

I'm not saying I can't see it in the future.  Maybe we can call -Wintrospection -Wall for now?

> This should be moved out of transformer.py and be a public function

But currently just the transformer knows what namespace it's processing.  We could potentially put this in each Node I guess...

> giscanner/log:warn(node, string, context) or so.
> 
> Can probably be more flexible, to allow the callsites of this function to do
> most of the string formatting.

Hmm...can you explain what caller would want to format things differently?
Comment 17 Colin Walters 2010-06-17 02:02:30 UTC
Attachment 163703 [details] pushed as 27b3465 - Add some more Gio annotations
Comment 18 Colin Walters 2010-06-17 02:04:56 UTC
I fixed up some more things and pushed, but am sure we are going to be further refining this, and am willing to do more on that.

The main problem as I see it right now is that the way AnnotationParser assigns defaults is a little hacky.  Mostly this is because we don't have a good way to say right now is_node_gobject(node) for example.

I'll probably be working on some transformer patches for this.
Comment 19 Johan (not receiving bugmail) Dahlin 2010-06-17 12:19:10 UTC
I ended up reverting the changeset because it broke the build. I also think there are a few thing that needs to be taken care of before this get pushed. I had another review at it before reverting. It obviously needs a bit more work.

- update the man page
- Use -Wall,-Werror, no double dashes, makes no sense to follow the gcc syntax if it's not exactly the same
- Don't use def func(foo=[]), it's always a bug
- Don't call private methods on foreign objects, eg self._transformer._xxx
- _log_symbol_warnings is unused and should be removed.
- don't use "string" as a variable name as it's a module
- don't use double underscores for private variables
- don't add trailing spaces in girparser.c

The build error we're seeing at the litl buildbot is the following:

Traceback (most recent call last):
  • File "../tools/g-ir-scanner", line 38 in <module>
    sys.exit(scanner_main(sys.argv))
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/scannermain.py", line 339 in scanner_main
    glibtransformer.final_analyze()
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 1156 in final_analyze
    self._walk(node, self._introspectable_pass2, [])
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 873 in _walk
    _subwalk(meth)
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 852 in _subwalk
    self._walk(subnode, callback, chain)
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 854 in _walk
    _subwalk(node.retval)
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 852 in _subwalk
    self._walk(subnode, callback, chain)
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 848 in _walk
    if not callback(node, chain):
  • File "/var/lib/buildbot/buildtmp/fox-gjs-update/build/gobject-introspection/giscanner/glibtransformer.py", line 1146 in _introspectable_pass2
    if target and not target.introspectable:
AttributeError: 'GLibObject' object has no attribute 'introspectable'

Comment 20 Colin Walters 2010-06-17 13:22:56 UTC
(In reply to comment #19)

> 
> The build error we're seeing at the litl buildbot is the following:
> 
> Traceback (most recent call last):

That's the cache.  Every time I get a bizarre python error I've trained myself to try rm -rf ~/.cache/g-ir-scanner
Comment 21 Johan (not receiving bugmail) Dahlin 2010-06-17 13:31:04 UTC
I did add versioning to the cache but there was a bug there which prevented the old files from being cleared out. Should be fixed now, eg, there should never be a reason to manually kill ~/.cache/g-ir-scanner any longer.
Comment 22 Colin Walters 2010-06-17 13:55:10 UTC
(In reply to comment #19)

> - update the man page

Okay...really we should be generating the man page from the optparse data somehow, having all this stuff in two places is rather a fail.

> - Use -Wall,-Werror, no double dashes, makes no sense to follow the gcc syntax
> if it's not exactly the same

I can't, optparse won't let me.  I'd rather pick a different format honestly.

--warn-all --warn-errors?

> - Don't use def func(foo=[]), it's always a bug

I don't see why it's *always* a bug.  You just have to be careful.  But okay, I changed it.

> - Don't call private methods on foreign objects, eg self._transformer._xxx

Okay, I made it public.

> - _log_symbol_warnings is unused and should be removed.

Hm, I was going to use it somewhere but I forgot where now.  I'll look.

> - don't use "string" as a variable name as it's a module

Okay.

> - don't use double underscores for private variables

I changed it.

> - don't add trailing spaces in girparser.c

Fixed.
Comment 23 Johan (not receiving bugmail) Dahlin 2010-06-17 14:15:05 UTC
(In reply to comment #22)
> (In reply to comment #19)
> 
> > - update the man page
> 
> Okay...really we should be generating the man page from the optparse data
> somehow, having all this stuff in two places is rather a fail.

Good idea. Do you want to integrate
http://andialbrecht.wordpress.com/2009/03/17/creating-a-man-page-with-distutils-and-optparse/ (or another variant) into the build?

Guess we still need to maintain the man pages for the tools written in C.

> > - Use -Wall,-Werror, no double dashes, makes no sense to follow the gcc syntax
> > if it's not exactly the same
> 
> I can't, optparse won't let me.  I'd rather pick a different format honestly.
> 

Yes you can, I thought I explained how to do this in a comment, here is a bit clearer code for it:

  parser.add_option('-W', action="append", dest="warnings", default=[])
Comment 24 Colin Walters 2010-06-17 14:18:03 UTC
(In reply to comment #23)
> 
> Yes you can, I thought I explained how to do this in a comment, here is a bit
> clearer code for it:
> 
>   parser.add_option('-W', action="append", dest="warnings", default=[])

But if I do that, then I can't document -Wall and -Werror.
Comment 25 Colin Walters 2010-06-17 14:18:29 UTC
Created attachment 163917 [details] [review]
Support introspectable=no attribute, add warnings framework

This work allows us to move closer to replacing gtk-doc, among other
things.  We add a generic attribute "introspectable", and inside the
typelib compiler if we see "introspectable=no", we don't put it in the
typelib.  This replaces the hackish pre-filter for varargs with a much
more generic mechanism.

The varargs is now handled in the scanner, and we emit
introspectable=no for them.

Add generic metadata to Node with references to file/line/column,
which currently comes from symbols.

Add scanner options --warn-all and --warn-error.
Comment 26 Johan (not receiving bugmail) Dahlin 2010-06-17 14:20:55 UTC
You can probably document it by adding/modifying the help formatter where you can append the -Wxxx options.
Comment 27 Colin Walters 2010-06-17 14:22:07 UTC
I decided to just use --warn-all and --warn-error, it's actually more confusing to follow GCC I think because people might think we pick up CFLAGS etc.
Comment 28 Colin Walters 2010-06-17 17:08:13 UTC
Attachment 163917 [details] pushed as 5a4fa2b - Support introspectable=no attribute, add warnings framework
Comment 29 André Klapper 2015-02-07 16:56:35 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]