Bug 120586 – [GTK] Make symbol export filter more strict, and disable for dev/test builds

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gustavo Noronha (kov)]

[GTK] Make symbol export filter more strict, and disable for dev/test builds

[Gustavo Noronha (kov)]

Created attachment 210255 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 210255 [details]
Patch

I really like the way this patch is going, but I have a few suggestions.

- I think the name of the flag should be a bit more general/descriptive since it doesn't just adjust the symbol lists. It also disables and enables test harnesses. Thus, I want to suggest something like --enable-developer-mode, but I'm not really attached to any name in particular.
- It would be interesting if this flag eventually was able to correctly enable all features that build-webkit sets in WebKitFeatureOverrides.txt, but I don't think this patch has to do that.
- Instead of the fine grained disabling of things that require hidden symbols, let's just use the developer mode to disable all testing harnesses. This will make builds faster for downstream and should make the build simpler.

What do you think?

[Zan Dobersek]

(In reply to comment #3)
> (From update of attachment 210255 [details])
> - Instead of the fine grained disabling of things that require hidden symbols, let's just use the developer mode to disable all testing harnesses. This will make builds faster for downstream and should make the build simpler.

This might be achievable by conditionally including sub-GNUmakefile.am files in the root one, presupposing that the testing-oriented build targets are dispersed among a manageable set of these.

This way, in non-developer mode, you'd only be building WTF, JSC, Platform, WebCore and WebKit/WebKit2 libraries, along with the jsc, WebKitPluginProcess, WebKitWebProcess and GtkLauncher/MiniBrowser programs and other build-time dependency executables (LLintOffsetExtractor etc.).

[Zan Dobersek]

Also, by 'disable all testing harnesses', do you mean the complete WebKit1 and WebKit2 unit test suites as well?

[Martin Robinson]

(In reply to comment #5)
> Also, by 'disable all testing harnesses', do you mean the complete WebKit1 and WebKit2 unit test suites as well?

If downstream typically runs them, no. If they don't, yes. :)

[Gustavo Noronha (kov)]

(In reply to comment #6)
> (In reply to comment #5)
> > Also, by 'disable all testing harnesses', do you mean the complete WebKit1 and WebKit2 unit test suites as well?
> 
> If downstream typically runs them, no. If they don't, yes. :)

In Debian we currently don't, but I would like to start running tests in the near future, though. I am not attached to the tests we have upstream, though, I can write simpler ones just for the package (it would be just to catch major breakages anyway).

I considered attaching the building of tests to --enable-maintainer-mode, the main issue with that would be most people actually build with maintainer mode enabled heh. In any case, I like the idea of using conditional inclusion of makefiles, and I like the idea of using --enable-developer-mode. I'll fix the patch up probably early next week, sorry for the delay.

[Gustavo Noronha (kov)]

Comment on attachment 210255 [details]
Patch

Removing the review flag meanwhile.

[Zan Dobersek]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Also, by 'disable all testing harnesses', do you mean the complete WebKit1 and WebKit2 unit test suites as well?
> > 
> > If downstream typically runs them, no. If they don't, yes. :)
> 
> In Debian we currently don't, but I would like to start running tests in the near future, though. I am not attached to the tests we have upstream, though, I can write simpler ones just for the package (it would be just to catch major breakages anyway).

I think downstream maintainers (or anyone building from the stable branch or a release tarball) shouldn't have to run the unit tests, but should be able to. I would also advise to not duplicate the testing effort.

Obviously great care is taken that the stable branch is passing all the unit tests, and I think this is being managed quite well in upstream. Because of that, on the assumption that the developer mode is introduced, I think it should be possible to disable all the unit tests when the developer mode is disabled.

[Gustavo Noronha (kov)]

Created attachment 211731 [details]
Patch

[Gustavo Noronha (kov)]

The gtk-wk2 EWS failure is a bit weird, I can't reproduce and it seems to fail building the wk1 library which built fine in the gtk bot.

[Zan Dobersek]

Comment on attachment 211731 [details]
Patch

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

> Tools/GNUmakefile.am:7
>  if ENABLE_WEBKIT1
>  noinst_PROGRAMS += \
> -	Programs/DumpRenderTree \
> -	Programs/GtkLauncher
> +	Programs/DumpRenderTree
>  endif

GtkLauncher target guarded with ENABLE_WEBKIT1 here ...

> Tools/GtkLauncher/GNUmakefile.am:2
> +noinst_PROGRAMS += \
> +	Programs/GtkLauncher

... but not here ...

> GNUmakefile.am:203
> +include Tools/gtk/GNUmakefile.am

... or here.

GtkLauncher pulls in the libwebkitgtk.so dependency, hence the linking in WK2-only builds. The GtkLauncher target should be guarded, though, and not this GNUmakefile.am inclusion, just like the WK1 unit tests.

I only ran through this patch to find the issue, haven't applied it and inspected the generated GNUmakefile, so there might be more to the problem.

[Gustavo Noronha (kov)]

Makes sense! I will fix that and try the patch out with wk1 disabled too.

[Gustavo Noronha (kov)]

Created attachment 211769 [details]
Patch

[Martin Robinson]

Comment on attachment 211769 [details]
Patch

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

> Tools/ChangeLog:18
> +        * gtk/common.py: do not use the Scripts directory as a reference to the top path;
> +        it's not necessary and it will not work now that the Scripts directory is not
> +        shipped in the tarball.

Is this true? I see VCSUtils.pm, for example, is still listed in EXTRA_DIST.

[Gustavo Noronha (kov)]

(In reply to comment #15)
> (From update of attachment 211769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211769&action=review
> 
> > Tools/ChangeLog:18
> > +        * gtk/common.py: do not use the Scripts directory as a reference to the top path;
> > +        it's not necessary and it will not work now that the Scripts directory is not
> > +        shipped in the tarball.
> 
> Is this true? I see VCSUtils.pm, for example, is still listed in EXTRA_DIST.

It's listed in an EXTRA_DIST that is inside a makefile that is not included when developer mode is off, so it doesn't end up in the tarball.

[Martin Robinson]

(In reply to comment #16)

> It's listed in an EXTRA_DIST that is inside a makefile that is not included when developer mode is off, so it doesn't end up in the tarball.

Makes sense. Thanks!

[Gustavo Noronha (kov)]

Comment on attachment 211769 [details]
Patch

Clearing flags on attachment: 211769

Committed r155918: <http://trac.webkit.org/changeset/155918>

[Gustavo Noronha (kov)]

All reviewed patches have been landed.  Closing bug.