Bug 103220

Created attachment 175938 [details] Patch

Some dependencies like ANGLE are actually checked into the tree. In the work I've been doing on IndexedDB I did this [1]. I imagine that the requirement for using a shared library version of leveldb would be that it has a stable API and that WebKit does not require new versions frequently. 1. https://github.com/mrobinson/webkit/tree/indexeddatabase-for-jsc

Comment on attachment 175938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175938&action=review Thanks for the patch! I've some doubts about the jhbuild moduleset changes and the implications of enabling indexdb. > Source/WebCore/ChangeLog:8 > + Tests: storage/indexeddb/* Shouldn't we unskip them? Have you checked all indexdb tests actually pass? > Tools/ChangeLog:9 > + * gtk/jhbuild.modules: > + * gtk/patches/leveldb-1.6.0-make-install.patch: Added. I think you should explain here why we need to build leveldb in our jhbuild, and why it requires a patch. Do different versions of leveldb give different results of layout tests? would it be possible to run the tests with the leveldb installed by the distro? > Tools/gtk/jhbuild.modules:23 > + <dep package="leveldb"/> This means we will always build leveldb even when indexdb is not enabled. I think there's no easy way to avoid it, so I guess it would be better to add to the moduleset dependencies that need to be built always and require a particular version to get the same tests results. > configure.ac:1030 > +if test "$enable_indexed_database" = "yes"; then > + PKG_CHECK_MODULES([LEVELDB], [leveldb]) > + AC_SUBST([LEVELDB_CFLAGS]) > + AC_SUBST([LEVELDB_LIBS]) > +fi This depends on a patch applied to leveldb by our jhbuild which means that it will always fail when not using the internal jhbuild + build-webkit even if leveldb is installed. I think you should use AC_SEARCH_LIBS() when PKG_CHECK_MODULES() fails to find the library.

(In reply to comment #3) > (From update of attachment 175938 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175938&action=review > > Thanks for the patch! I've some doubts about the jhbuild moduleset changes and the implications of enabling indexdb. > > > Source/WebCore/ChangeLog:8 > > + Tests: storage/indexeddb/* > > Shouldn't we unskip them? Have you checked all indexdb tests actually pass? IndexedDB isn't finished for JSC ports, so tests won't pass until that's complete. I don't think this patch actually enables IndexedDB. We likely shouldn't put LevelDB in the jhbuild moduleset until we've enabled IndexedDB and are running tests.

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 175938 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175938&action=review > > > > Thanks for the patch! I've some doubts about the jhbuild moduleset changes and the implications of enabling indexdb. > > > > > Source/WebCore/ChangeLog:8 > > > + Tests: storage/indexeddb/* > > > > Shouldn't we unskip them? Have you checked all indexdb tests actually pass? > > IndexedDB isn't finished for JSC ports, so tests won't pass until that's complete. I don't think this patch actually enables IndexedDB. We likely shouldn't put LevelDB in the jhbuild moduleset until we've enabled IndexedDB and are running tests. + Charles, who is doing this work.

I was just looking at this for EFL... (In reply to comment #3) > I think you should explain here why we need to build leveldb in our jhbuild, and why it requires a patch. Do different versions of leveldb give different results of layout tests? would it be possible to run the tests with the leveldb installed by the distro? You guys maybe handled this in other channels, but for reference: From what I've seen so far, the big stumbling block is helpers/memenv/memenv.h which is apparently not considered part of the library, but is used by many leveldb users. Otherwise it looks decent if not aimed at typical shared distribution (Makefile doesn't even have an install target). We could just copy-paste the memenv helper (< 380 lines of C++) into webkit and use distro/jhbuild leveldb, right? > > configure.ac:1030 > > +if test "$enable_indexed_database" = "yes"; then > > + PKG_CHECK_MODULES([LEVELDB], [leveldb]) > > + AC_SUBST([LEVELDB_CFLAGS]) > > + AC_SUBST([LEVELDB_LIBS]) > > +fi > > This depends on a patch applied to leveldb by our jhbuild which means that it will always fail when not using the internal jhbuild + build-webkit even if leveldb is installed. I think you should use AC_SEARCH_LIBS() when PKG_CHECK_MODULES() fails to find the library. Is your leveldb patch somewhere available? I was just patching it for testing as well and it would make sense if we were using the same patch...

(In reply to comment #6) > > This depends on a patch applied to leveldb by our jhbuild which means that it will always fail when not using the internal jhbuild + build-webkit even if leveldb is installed. I think you should use AC_SEARCH_LIBS() when PKG_CHECK_MODULES() fails to find the library. > > Is your leveldb patch somewhere available? I was just patching it for testing as well and it would make sense if we were using the same patch... NM, I understood you mean the one in this patch.

(In reply to comment #5) > This depends on a patch applied to leveldb by our jhbuild which means that it will always fail when not using the internal jhbuild + build-webkit even if leveldb is installed. One more comment to this: because of the memenv issue I mentioned, the build will fail with distro leveldb anyway.

(In reply to comment #8) > (In reply to comment #5) > > This depends on a patch applied to leveldb by our jhbuild which means that it will always fail when not using the internal jhbuild + build-webkit even if leveldb is installed. > > One more comment to this: because of the memenv issue I mentioned, the build will fail with distro leveldb anyway. Yes, I was thinking about this issue recently and I think that we should try first to use the distribution leveldb (which is installed as a .a file on my system) and then add memenv to the ThirdParty directory (assuming the license is compatible). I'll try to write a patch like this today.

I believe that the approach I have taken with this patch of building a captive library is the most maintainable despite requiring compiling LevelDB as part of building WebKit. This approach avoids the not inconsiderable burden of ensuring consistency across distributions in API revisions and in compilation settings for a library with a C++ interface.

(In reply to comment #10) > I believe that the approach I have taken with this patch of building a captive library is the most maintainable despite requiring compiling LevelDB as part of building WebKit. This approach avoids the not inconsiderable burden of ensuring consistency across distributions in API revisions and in compilation settings for a library with a C++ interface. The issue is that the jhbuild modules are only used by developers.

(In reply to comment #11) > (In reply to comment #10) > > I believe that the approach I have taken with this patch of building a captive library is the most maintainable despite requiring compiling LevelDB as part of building WebKit. This approach avoids the not inconsiderable burden of ensuring consistency across distributions in API revisions and in compilation settings for a library with a C++ interface. > > The issue is that the jhbuild modules are only used by developers. To expand on this, if we want to ship a version of leveldb to downstream, we probably need to bundle it in the ThirdParty directory. My plan for today was to only do this for memenv, but if you feel that it's important to do this for all of leveldb, I can write a patch that does that too.

(In reply to comment #12) > > > > The issue is that the jhbuild modules are only used by developers. > > To expand on this, if we want to ship a version of leveldb to downstream, we probably need to bundle it in the ThirdParty directory. My plan for today was to only do this for memenv, but if you feel that it's important to do this for all of leveldb, I can write a patch that does that too. Yes. This is what I was saying in email as well: The people building operating systems are likely to strangle us if we tell them that building webkit requires a modified 3rd party project. I vote for bundling memenv code and relying on jhbuild/distro for leveldb. I'll write to leveldb upstream just to check that this is a sane idea and what they intended memenv to be used for (that's what I assume) -- let me know if you happen to have confirmed this already or know the code well enough that this is not needed. I'll also try to get an install target in the Makefile and a pc file included in there so we wouldn't have to patch in future. I'll CC you guys.

(In reply to comment #6) > From what I've seen so far, the big stumbling block is helpers/memenv/memenv.h which is apparently not considered part of the library, but is used by many leveldb users. Otherwise it looks decent if not aimed at typical shared distribution (Makefile doesn't even have an install target). I don't know if it's used by many leveldb users. We added it to leveldb for chrome's incognito mode. Do GTK or EFL support something similar? If not, we could #ifdef chromium around memenv and then you won't have to worry about it.

(In reply to comment #14) > (In reply to comment #6) > > > From what I've seen so far, the big stumbling block is helpers/memenv/memenv.h which is apparently not considered part of the library, but is used by many leveldb users. Otherwise it looks decent if not aimed at typical shared distribution (Makefile doesn't even have an install target). > > I don't know if it's used by many leveldb users. We added it to leveldb for chrome's incognito mode. Do GTK or EFL support something similar? If not, we could #ifdef chromium around memenv and then you won't have to worry about it. If we add leveldb to Sources/ThirdParty, memenv shouldn't be an issue. This seems to be the way to go since WebKit needs to depend on a particular version (which distributions might not ship). Can you comment on the amount of API/ABI-breaking changes that happen in leveldb and WebKit's IDB implementation?

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #6) > > > > > From what I've seen so far, the big stumbling block is helpers/memenv/memenv.h which is apparently not considered part of the library, but is used by many leveldb users. Otherwise it looks decent if not aimed at typical shared distribution (Makefile doesn't even have an install target). > > > > I don't know if it's used by many leveldb users. We added it to leveldb for chrome's incognito mode. Do GTK or EFL support something similar? If not, we could #ifdef chromium around memenv and then you won't have to worry about it. > > If we add leveldb to Sources/ThirdParty, memenv shouldn't be an issue. This seems to be the way to go since WebKit needs to depend on a particular version (which distributions might not ship). Agreed that including a full copy of leveldb in ThirdParty is the way to go. > Can you comment on the amount of API/ABI-breaking changes that happen in leveldb and WebKit's IDB implementation? There's been one API change in the past year. Code using the old API would not work with the new API but code using the new API would work with the old API. Does that fit this definition of "breaking"? I don't know much about ABI changes, we always recompile. ABI compatibility has never been considered a goal of the leveldb project, though.

(In reply to comment #16) > (In reply to comment #15) > > If we add leveldb to Sources/ThirdParty, memenv shouldn't be an issue. This seems to be the way to go since WebKit needs to depend on a particular version (which distributions might not ship). > > Agreed that including a full copy of leveldb in ThirdParty is the way to go. Can you guys explain why this is -- why we need to depend on a specific version? Didn't David just say the API is backwards compatible (or at least has been historically)? I'm fine with bundling whole leveldb as well, it's a lot better than breaking on dependency updates. But I also know distributors would be happiest if we can use the libraries they provide.

(In reply to comment #17) > Can you guys explain why this is -- why we need to depend on a specific version? Didn't David just say the API is backwards compatible (or at least has been historically)? Shouldn't comment before coffee -- didn't notice the ABI part. I guess if soname isn't guaranteed to change when it should, then copypasta is the only option.

Great. I have a patch in my working copy which I expect to post today.

Created attachment 182169 [details] Patch

(In reply to comment #20) > Created an attachment (id=182169) [details] > Patch Sorry, Michael. I didn't actually mean to obsolete your patch, but I including leveldb in ThirdParty is our best bet for the future.

Created attachment 183197 [details] Patch

Comment on attachment 183197 [details] Patch OK

Comment on attachment 183197 [details] Patch Clearing flags on attachment: 183197 Committed r140021: <http://trac.webkit.org/changeset/140021>

All reviewed patches have been landed. Closing bug.