Bug 129571 – [GTK] Simplify the GObject DOM bindings API break check into one step

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Right now the API break check is a couple scripts with somewhat tricky integration into the two builds. We should simplify the check into one script and also move it to 'make check.'

[Carlos Garcia Campos]

We can't make make check fail for API incompatible changes because of bug #121481. I *always* get incompatible changes when running make distcheck, because the symbols file used as reference is only valid for the compile options used by build-webkit. However, we want to always run the script for every build, to early detect API changes.

[Carlos Garcia Campos]

hmm, what other scripts do you mean? there's only one, gobject-run-api-break-test

[Martin Robinson]

(In reply to comment #1)
> We can't make make check fail for API incompatible changes because of bug #121481. I *always* get incompatible changes when running make distcheck, because the symbols file used as reference is only valid for the compile options used by build-webkit. However, we want to always run the script for every build, to early detect API changes.

If we can't run the script during "check" why can we run it for every build? If it runs for every build, won't it also run when you do "make distcheck?"

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #1)
> > We can't make make check fail for API incompatible changes because of bug #121481. I *always* get incompatible changes when running make distcheck, because the symbols file used as reference is only valid for the compile options used by build-webkit. However, we want to always run the script for every build, to early detect API changes.
> 
> If we can't run the script during "check" why can we run it for every build? If it runs for every build, won't it also run when you do "make distcheck?"

I haven't said we can't run it, what I've said is that we can't make make check fail. And I think we don't need to move it to make check, but simply run it always.

[Martin Robinson]

(In reply to comment #4)
 
> I haven't said we can't run it, what I've said is that we can't make make check fail. And I think we don't need to move it to make check, but simply run it always.

Yes, the script doesn't return a failing error code ever, right now because of the bug you linked to. The reason I suggest moving it to make check is that after the simplification, it doesn't produce any output file (the current output file is unused and is used more like a stamp). Parts of the build that are tests (ie is the API broken?) and don't produce any output are more like tests. Since the EWS runs "make check" already it seemed okay. If you like though, I can make it part of 'build-webkit' instead. I don't think it makes sense to keep it as part of the default target though, because it doesn't produce any output.

[Carlos Garcia Campos]

I see your point and what you say makes a lot of sense in theory, but in practice things are a bit different. First, are your sure EWS run tests? since the script can't fail because of bug #121481, the EWS box will be green in any case and nobody will notice it. The same happens for the bots running the tests, although in that case we could make the script fail. So, if we add a new step of the bots that simply run this script and we make it fail, I would be happy to remove it from the normal target. Meanwhile it's the only way I have to early detect api breaks in DOM bindings.

[Martin Robinson]

Created attachment 225721 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 225721 [details]
Patch

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

I think this breaks distcheck. I think the patch could be split, one patch to move all the logic to a single script (like it was originally) and another one to move it to make check. I really want to make sure I'm not going to miss the api breaks.

> Source/WebCore/bindings/gobject/GNUmakefile.am:-503
> -DerivedSources/webkitdom/webkitdom.symbols: $(gdom_symbol_files) $(WebCore)/bindings/gobject/webkitdom.symbols $(WebCore)/bindings/scripts/gobject-run-api-break-test $(srcdir)/Tools/gtk/check-gdom-symbols
> -	$(AM_V_GEN)$(srcdir)/Tools/gtk/check-gdom-symbols
> -

I didn't even know this script existed. Anyway, you should also remove the webkitdom.symbols file from EXTRA_DIST

> Source/WebCore/bindings/scripts/gobject-run-api-break-test:-1
> -#!/usr/bin/env python

You should also remove the script from EXTRA_DIST in Source/WebCore/GNUmakefile.am

> Tools/ChangeLog:9
> +        * gtk/check-for-webkitdom-api-breaks: Added. A combination of the two removed scripts.

Why the rename? what was wrong with gobject-run-api-break-test? I really don't mind the name, I'm just curious.

> Tools/GNUmakefile.am:270
> +	$(top_srcdir)/Tools/gtk/check-for-webkitdom-api-breaks

You should add it to EXTRA_DIST too. Are you sure this is run by the bots? I'm afraid I'm going to miss all api breaks from now on. Would it be possible to add a specific step in the bots for this particular test, like the other tests?

> Tools/gtk/check-for-webkitdom-api-breaks:44
> +    missing_api = expected_api.difference(built_api)
> +    new_api = built_api.difference(expected_api)

I love this approach of using sets and get the difference, it's much better than having to parse diffs :-)

> Tools/gtk/check-for-webkitdom-api-breaks:52
> +        sys.stderr.write("New API detected in GObject DOM bindings\n")
> +        sys.stderr.write("    %s\n" % "    ".join(new_api))

This is not an error, we used to write this to stdout instead.

> Tools/gtk/webkitdom.py:1
> +#!/usr/bin/env python

This new file should be added to EXTRA_DIST as well

> Tools/gtk/webkitdom.py:46
> +        if header_name == "webkitdom.h" or header_name == "webkitdomdefines.h":

if header_name in ("webkitdom.h", "webkitdomdefines.h"):

[Martin Robinson]

(In reply to comment #8)
> (From update of attachment 225721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225721&action=review

> > Source/WebCore/bindings/gobject/GNUmakefile.am:-503
> > -DerivedSources/webkitdom/webkitdom.symbols: $(gdom_symbol_files) $(WebCore)/bindings/gobject/webkitdom.symbols $(WebCore)/bindings/scripts/gobject-run-api-break-test $(srcdir)/Tools/gtk/check-gdom-symbols
> > -	$(AM_V_GEN)$(srcdir)/Tools/gtk/check-gdom-symbols
> > -
> 
> I didn't even know this script existed. Anyway, you should also remove the webkitdom.symbols file from EXTRA_DIST

The webkitdom.symbols that's on EXTRA_DIST is the one included in the source tree, which is still in use.

> > Source/WebCore/bindings/scripts/gobject-run-api-break-test:-1
> > -#!/usr/bin/env python
> 
> You should also remove the script from EXTRA_DIST in Source/WebCore/GNUmakefile.am

Nice catch. Done.

> > Tools/ChangeLog:9
> > +        * gtk/check-for-webkitdom-api-breaks: Added. A combination of the two removed scripts.
> 
> Why the rename? what was wrong with gobject-run-api-break-test? I really don't mind the name, I'm just curious.

Typically scripts in WebKit are named so that they parse as English sentences. An example here is "run-webkit-tests" and "generate-gtkdoc." Having the "gobject" before "check" means that you cannot read it like a command sentence. I think the scripts in the bindings directory only have that so they stick out from the non-GObject ones. I'm not sure I agree it's a great pattern anyhow. :)

> > Tools/GNUmakefile.am:270
> > +	$(top_srcdir)/Tools/gtk/check-for-webkitdom-api-breaks
> 
> You should add it to EXTRA_DIST too. Are you sure this is run by the bots? I'm afraid I'm going to miss all api breaks from now on. Would it be possible to add a specific step in the bots for this particular test, like the other tests?

Nice catch. Added. I have opened a bug and attached a buildbot patch already. See: https://bugs.webkit.org/show_bug.cgi?id=129637

> 
> > Tools/gtk/check-for-webkitdom-api-breaks:44
> > +    missing_api = expected_api.difference(built_api)
> > +    new_api = built_api.difference(expected_api)
> 
> I love this approach of using sets and get the difference, it's much better than having to parse diffs :-)

Thanks. :)


> > Tools/gtk/check-for-webkitdom-api-breaks:52
> > +        sys.stderr.write("New API detected in GObject DOM bindings\n")
> > +        sys.stderr.write("    %s\n" % "    ".join(new_api))
> 
> This is not an error, we used to write this to stdout instead.
> 
> > Tools/gtk/webkitdom.py:1
> > +#!/usr/bin/env python
> 
> This new file should be added to EXTRA_DIST as well

Done.

> > Tools/gtk/webkitdom.py:46
> > +        if header_name == "webkitdom.h" or header_name == "webkitdomdefines.h":
> 
> if header_name in ("webkitdom.h", "webkitdomdefines.h"):

Sure.

[Martin Robinson]

Created attachment 225781 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 225721 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225721&action=review
> 
> > > Source/WebCore/bindings/gobject/GNUmakefile.am:-503
> > > -DerivedSources/webkitdom/webkitdom.symbols: $(gdom_symbol_files) $(WebCore)/bindings/gobject/webkitdom.symbols $(WebCore)/bindings/scripts/gobject-run-api-break-test $(srcdir)/Tools/gtk/check-gdom-symbols
> > > -	$(AM_V_GEN)$(srcdir)/Tools/gtk/check-gdom-symbols
> > > -
> > 
> > I didn't even know this script existed. Anyway, you should also remove the webkitdom.symbols file from EXTRA_DIST
> 
> The webkitdom.symbols that's on EXTRA_DIST is the one included in the source tree, which is still in use.
>

Oh, you are right, this one is autogenerated and not disted, of course.

> > > Tools/ChangeLog:9
> > > +        * gtk/check-for-webkitdom-api-breaks: Added. A combination of the two removed scripts.
> > 
> > Why the rename? what was wrong with gobject-run-api-break-test? I really don't mind the name, I'm just curious.
> 
> Typically scripts in WebKit are named so that they parse as English sentences. An example here is "run-webkit-tests" and "generate-gtkdoc." Having the "gobject" before "check" means that you cannot read it like a command sentence. I think the scripts in the bindings directory only have that so they stick out from the non-GObject ones. I'm not sure I agree it's a great pattern anyhow. :)

Right, the gobject prefix was because the script was in bindings dir. So, I think run-gobject-webkitdom-api-break-test would be more consistent with other wk scripts.
 
> > > Tools/GNUmakefile.am:270
> > > +	$(top_srcdir)/Tools/gtk/check-for-webkitdom-api-breaks
> > 
> > You should add it to EXTRA_DIST too. Are you sure this is run by the bots? I'm afraid I'm going to miss all api breaks from now on. Would it be possible to add a specific step in the bots for this particular test, like the other tests?
> 
> Nice catch. Added. I have opened a bug and attached a buildbot patch already. See: https://bugs.webkit.org/show_bug.cgi?id=129637

Thanks!

[Carlos Garcia Campos]

Comment on attachment 225781 [details]
Patch

Oooook, let's see how this works :-)

[Martin Robinson]

Comment on attachment 225781 [details]
Patch

Clearing flags on attachment: 225781

Committed r165060: <http://trac.webkit.org/changeset/165060>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.