RESOLVED FIXED 103220
Import LevelDB to the ThirdParty directory for IndexedDB
https://bugs.webkit.org/show_bug.cgi?id=103220
Summary Import LevelDB to the ThirdParty directory for IndexedDB
Michael Pruett
Reported 2012-11-26 00:20:42 PST
WebKitGTK+ should be built with LevelDB when IndexedDB is enabled.
Attachments
Patch (9.10 KB, patch)
2012-11-26 00:45 PST, Michael Pruett
no flags
Patch (855.95 KB, patch)
2013-01-10 10:57 PST, Martin Robinson
no flags
Patch (856.04 KB, patch)
2013-01-17 09:07 PST, Martin Robinson
no flags
Michael Pruett
Comment 1 2012-11-26 00:45:31 PST
Martin Robinson
Comment 2 2012-11-26 08:59:52 PST
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
Carlos Garcia Campos
Comment 3 2012-11-27 00:16:54 PST
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.
Martin Robinson
Comment 4 2012-11-27 07:59:19 PST
(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.
Antonio Gomes
Comment 5 2012-11-27 11:04:33 PST
(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.
Jussi Kukkonen (jku)
Comment 6 2013-01-08 00:46:35 PST
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...
Jussi Kukkonen (jku)
Comment 7 2013-01-08 00:57:45 PST
(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.
Jussi Kukkonen (jku)
Comment 8 2013-01-08 01:11:28 PST
(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.
Martin Robinson
Comment 9 2013-01-08 08:40:54 PST
(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.
Michael Pruett
Comment 10 2013-01-08 10:24:35 PST
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.
Martin Robinson
Comment 11 2013-01-08 10:43:11 PST
(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.
Martin Robinson
Comment 12 2013-01-08 10:47:08 PST
(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.
Jussi Kukkonen (jku)
Comment 13 2013-01-09 01:03:04 PST
(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.
David Grogan
Comment 14 2013-01-09 14:42:25 PST
(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.
Martin Robinson
Comment 15 2013-01-09 14:54:07 PST
(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?
David Grogan
Comment 16 2013-01-09 15:18:58 PST
(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.
Jussi Kukkonen (jku)
Comment 17 2013-01-10 00:56:47 PST
(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.
Jussi Kukkonen (jku)
Comment 18 2013-01-10 01:16:41 PST
(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.
Martin Robinson
Comment 19 2013-01-10 05:17:39 PST
Great. I have a patch in my working copy which I expect to post today.
Martin Robinson
Comment 20 2013-01-10 10:57:58 PST
Martin Robinson
Comment 21 2013-01-10 11:07:02 PST
(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.
Martin Robinson
Comment 22 2013-01-17 09:07:03 PST
Gustavo Noronha (kov)
Comment 23 2013-01-17 09:20:28 PST
Comment on attachment 183197 [details] Patch OK
WebKit Review Bot
Comment 24 2013-01-17 12:04:11 PST
Comment on attachment 183197 [details] Patch Clearing flags on attachment: 183197 Committed r140021: <http://trac.webkit.org/changeset/140021>
WebKit Review Bot
Comment 25 2013-01-17 12:04:20 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.