Bug 101224 – [GTK] Check DOM bindings API compatibility while building

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Making the build fail if the API is broken.

[Carlos Garcia Campos]

Created attachment 172342 [details]
Patch

There's something I'm not sure about in this patch. It keeps a symbols file in the source tree to compare with the symbols generated during the build and if the changes are API comapatible the symbols file in the source tree is updated. I'm not sure it's a good idea that the build updates a file in the source tree. Most of the times the API changes, we don't notice it, so if the file has to be update manually, we will miss it most of the times. We could make the build fail even for compatible changes, to force update the symbols file, showing in the error message if the changes are an API break or not, so that it's easy to know whether we just need to update the file or we are breaking the API.

[Martin Robinson]

Comment on attachment 172342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172342&action=review

> Source/WebCore/bindings/gobject/GNUmakefile.am:484
> +	&& (cmp -s $@ $(WebCore)/bindings/gobject/webkitdom.symbols || $(PYTHON) $(WebCore)/bindings/scripts/gobject-check-api $@ $(WebCore)/bindings/gobject/webkitdom.symbols) \

I'm curious why you don't just use the diff tool here.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1484
> +    # Write a symbols file.
> +    my $symbolsFileName = "$outputDir/" . $basename . ".symbols";
> +    open(SYM, ">$symbolsFileName") or die "Couldn't open file $symbolsFileName";
> +    print SYM @symbols;
> +    close(SYM);

Instead of four symbols files, I wonder if there could only be one.

> Source/WebCore/bindings/scripts/gobject-check-api:1
> +#!/usr/bin/env python

Just a naming nit: This should probably be called detect-gobject-api-breaks or something like that, especially with the verb first.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 172342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172342&action=review
> 
> > Source/WebCore/bindings/gobject/GNUmakefile.am:484
> > +	&& (cmp -s $@ $(WebCore)/bindings/gobject/webkitdom.symbols || $(PYTHON) $(WebCore)/bindings/scripts/gobject-check-api $@ $(WebCore)/bindings/gobject/webkitdom.symbols) \
> 
> I'm curious why you don't just use the diff tool here.

I'm assuming it's faster to use cmp when the files are the same than running the python script.

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1484
> > +    # Write a symbols file.
> > +    my $symbolsFileName = "$outputDir/" . $basename . ".symbols";
> > +    open(SYM, ">$symbolsFileName") or die "Couldn't open file $symbolsFileName";
> > +    print SYM @symbols;
> > +    close(SYM);
> 
> Instead of four symbols files, I wonder if there could only be one.

There aren't four, there's one symbols file per DOM object. This is because the generator script is called per DOM object. We could use a single file, and append the symbols, but then we would need rules in the makefile to delete the file before generating the bindings, and append the symbols for the static sources, so it would be more complicated. 

Also, I plan to use this to create a -sections.txt file for gtk-doc. Having a .symbols file per object makes it easier, because we know that every .symbols files is a section, and its contents the methods.

> > Source/WebCore/bindings/scripts/gobject-check-api:1
> > +#!/usr/bin/env python
> 
> Just a naming nit: This should probably be called detect-gobject-api-breaks or something like that, especially with the verb first.

I followed the same approach than the other bindings scripts like gobject-generate-headers.pl

[Martin Robinson]

(In reply to comment #3)
 
> > > Source/WebCore/bindings/gobject/GNUmakefile.am:484
> > > +	&& (cmp -s $@ $(WebCore)/bindings/gobject/webkitdom.symbols || $(PYTHON) $(WebCore)/bindings/scripts/gobject-check-api $@ $(WebCore)/bindings/gobject/webkitdom.symbols) \
> > 
> > I'm curious why you don't just use the diff tool here.
> 
> I'm assuming it's faster to use cmp when the files are the same than running the python script.

I was wondering if the entire script just be replaced entirely by careful execution of the diff command?

> I followed the same approach than the other bindings scripts like gobject-generate-headers.pl

Then perhaps gobject-detect-api-breaks would be a better name.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > > > Source/WebCore/bindings/gobject/GNUmakefile.am:484
> > > > +	&& (cmp -s $@ $(WebCore)/bindings/gobject/webkitdom.symbols || $(PYTHON) $(WebCore)/bindings/scripts/gobject-check-api $@ $(WebCore)/bindings/gobject/webkitdom.symbols) \
> > > 
> > > I'm curious why you don't just use the diff tool here.
> > 
> > I'm assuming it's faster to use cmp when the files are the same than running the python script.
> 
> I was wondering if the entire script just be replaced entirely by careful execution of the diff command?

I think diff always returns 1 when files are different, which doesn't necessarily mean an API break, only if there are lines removed (due to removal or modification of a symbol). I guess we could just run always the script instead of comparing twice.

> > I followed the same approach than the other bindings scripts like gobject-generate-headers.pl
> 
> Then perhaps gobject-detect-api-breaks would be a better name.

Ok, it's just a name.

[Martin Robinson]

(In reply to comment #3)

> There aren't four, there's one symbols file per DOM object. This is because the generator script is called per DOM object. We could use a single file, and append the symbols, but then we would need rules in the makefile to delete the file before generating the bindings, and append the symbols for the static sources, so it would be more complicated. 

This is not what I'm seeing in the patch. I see four .symbols files. One for EventTarget, one for DOMObject, one for the custom APIs and one for the rest of the DOM objects. It seems like this division is based on the internal details of how the bindings are generated.

I'm not sure that's the kind of division we want in the API docs. For instance, a section per DOM object or everything in one section seems more logical.

Perhaps it's cleaner to dump all symbols into the same file. The script could even operate like the bindings tests, which handles both the step of generating the files (via --reset-results) and failing if it changes otherwise.

Another idea is that we could just roll this into run-gtk-tests -- the pre-existing GTk+ API test suite. That's fewer build rules and 'make test' works automatically.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #3)
> 
> > There aren't four, there's one symbols file per DOM object. This is because the generator script is called per DOM object. We could use a single file, and append the symbols, but then we would need rules in the makefile to delete the file before generating the bindings, and append the symbols for the static sources, so it would be more complicated. 
> 
> This is not what I'm seeing in the patch. I see four .symbols files. One for EventTarget, one for DOMObject, one for the custom APIs and one for the rest of the DOM objects. It seems like this division is based on the internal details of how the bindings are generated.

DOM bindings are a collection of wrapper objects, most of them are auto-generated at build time and two (EvenTarget and Object) are static, manually written and are in the source tree. Custom is a special case because it's not a DOM object, but it contains public methods too. 

This division is an implementation detail, in the end what we have is a collection of objects like any other API. If you look at the current Makefile, we have to manually copy the static sources to the derived sources dir during building so that all files are handled the same way (it makes everything easier). 

I'm doing the same here, for autogenerated objects, the .symbols file is generated by the same script that creates the code, and for static code, the .symbols file is created manually. In the end, we have a .symbols file per object (and one for custom), so we don't need to handle the static code as a separate case anymore.

When all bindings have been generated, a .symbols file is created concatenating all .symbols file, and it's used to compare with the .symbols file we keep in the source tree (all others are temporary files in the build dir). This is one of the goals of the goals of this. If there's an API break (not necessarily any change in the API but only an API incompatible change) the build finishes early showing a diff between the .symbols file in the source tree and the concatenation of all other .symbols files.

> I'm not sure that's the kind of division we want in the API docs. For instance, a section per DOM object or everything in one section seems more logical.

A -sections.txt file contains a section per object in the API, something like this:

<SECTION>
<FILE>WebKitDOMDocument</FILE>
<TITLE>WebKitDOMDocument</TITLE>
WebKitDOMDocument
webkit_dom_document_create_element
webkit_dom_document_create_document_fragment
....

<SUBSECTION Standard>
WEBKIT_TYPE_DOM_DOCUMENT
WEBKIT_DOM_DOCUMENT
WEBKIT_DOM_IS_DOCUMENT
WEBKIT_DOM_DOCUMENT_CLASS
WEBKIT_DOM_IS_DOCUMENT_CLASS

<SUBSECTION Private>
webkit_dom_document_get_type
</SECTION>
.....

This file doesn't necessarily means there will be a section per object in the docs, that's organized in the docs.sgml file. We could have a whole DOM bindings subsection in the docs with all the DOM API. Please, look at other gtk-doc projects or how we create the docs for WebKit1 and WEbKit2 APIs.

> Perhaps it's cleaner to dump all symbols into the same file. The script could even operate like the bindings tests, which handles both the step of generating the files (via --reset-results) and failing if it changes otherwise.

Yes, I thought about adding --reset-results to gobject-check-api, to manually update the results, but I'm still not sure how to handle the results update yet, see comment #1.

> Another idea is that we could just roll this into run-gtk-tests -- the pre-existing GTk+ API test suite. That's fewer build rules and 'make test' works automatically.

But we don't want to break make test or make check, but make all, and as soon as possible.

[Carlos Garcia Campos]

Created attachment 172572 [details]
Updated patch

I've renamed the script and added --reset-results command line option so that it never updates the symbols file in the source tree during building. In case of API compatible changes, it shows the diff and suggests to update the results executing the script with --reset-results option, but the build continues. As in previous patch in case of incompatible API changes the build finishes.

[Luis de Bethencourt]

Created attachment 194681 [details]
rebased patch, applies cleanly

I've rebased the patch so it applies cleanly to master now.

Thanks Carlos! :)

[Philippe Normand]

So which patch should be reviewed? I assume the rebased one?

[kov's GTK+ EWS bot]

Comment on attachment 194681 [details]
rebased patch, applies cleanly

Attachment 194681 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/139182

[Philippe Normand]

Comment on attachment 194681 [details]
rebased patch, applies cleanly

View in context: https://bugs.webkit.org/attachment.cgi?id=194681&action=review

> Source/WebCore/bindings/scripts/gobject-run-api-break-test:29
> +if sys.argv[1] == '--reset-results':
> +    reset_results = True
> +    from_file = sys.argv[2]
> +    to_file = sys.argv[3]
> +else:
> +    reset_results = False
> +    from_file = sys.argv[1]
> +    to_file = sys.argv[2]

Well, arguments handling is usually done with getopt, I think we should use it here.

[Philippe Normand]

Comment on attachment 194681 [details]
rebased patch, applies cleanly

View in context: https://bugs.webkit.org/attachment.cgi?id=194681&action=review

> Source/WebCore/bindings/scripts/gobject-run-api-break-test:63
> +sys.exit(0)

This can be removed :)

[Carlos Garcia Campos]

(In reply to comment #12)
> (From update of attachment 194681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194681&action=review
> 
> > Source/WebCore/bindings/scripts/gobject-run-api-break-test:29
> > +if sys.argv[1] == '--reset-results':
> > +    reset_results = True
> > +    from_file = sys.argv[2]
> > +    to_file = sys.argv[3]
> > +else:
> > +    reset_results = False
> > +    from_file = sys.argv[1]
> > +    to_file = sys.argv[2]
> 
> Well, arguments handling is usually done with getopt, I think we should use it here.

A bit overkill when there's only one argument, and this script is not expected to be run by users, but still, I can implement it using argparse.

[Carlos Garcia Campos]

Created attachment 211753 [details]
Updated patch

I'm giving this another try. I've updated the patch to apply on current git master, updated the symbols file we keep in source tree and addressed review comments. I plan to use this symbols files also to generate API docs for GObject DOM bindings.

[Gustavo Noronha (kov)]

Comment on attachment 211753 [details]
Updated patch

Let's do this for this new cycle!

[Carlos Garcia Campos]

Committed r155850: <http://trac.webkit.org/changeset/155850>

[Alexey Proskuryakov]

This broke bindings tests on Mac <http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/12968/steps/bindings-generation-tests/logs/stdio>, so I'm going to roll out. Neither author nor reviewer appear to be on IRC.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121447

[Zan Dobersek]

Rebaselined the GObject bindings tests in r155893. Again closing.
https://trac.webkit.org/r155893

[Carlos Garcia Campos]

Sorry about the bindings test failure, I didn't expect this change to affect the tests, because I didn't know the test made a diff of the whole output directory, so now the .symbols files should also be included in the test expectations. Thanks Zan for the rebaseline. I was also surprised that mac bots were affected by GObject DOM bindings changes, maybe we could add options to run-bindings-tests to test only certain bindings, --objc --js for example.