[GTK] Make symbol export filter more strict, and disable for dev/test builds
Created attachment 210255 [details] Patch
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
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?
(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.).
Also, by 'disable all testing harnesses', do you mean the complete WebKit1 and WebKit2 unit test suites as well?
(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 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.
Comment on attachment 210255 [details] Patch Removing the review flag meanwhile.
(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.
Created attachment 211731 [details] Patch
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.
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.
Makes sense! I will fix that and try the patch out with wk1 disabled too.
Created attachment 211769 [details] Patch
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.
(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.
(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!
Comment on attachment 211769 [details] Patch Clearing flags on attachment: 211769 Committed r155918: <http://trac.webkit.org/changeset/155918>
All reviewed patches have been landed. Closing bug.