Bug 103220

Summary: Import LevelDB to the ThirdParty directory for IndexedDB
Product: WebKit Reporter: Michael Pruett <michael>
Component: New BugsAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, cgarcia, charles.wei, cmarcelo, dgrogan, dpranke, d-r, eric, jsbell, jussi.kukkonen, laszlo.gombos, levin, levin+threading, mrobinson, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98932, 106443    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Michael Pruett 2012-11-26 00:20:42 PST
WebKitGTK+ should be built with LevelDB when IndexedDB is enabled.
Comment 1 Michael Pruett 2012-11-26 00:45:31 PST
Created attachment 175938 [details]
Patch
Comment 2 Martin Robinson 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
Comment 3 Carlos Garcia Campos 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.
Comment 4 Martin Robinson 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.
Comment 5 Antonio Gomes 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.
Comment 6 Jussi Kukkonen (jku) 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...
Comment 7 Jussi Kukkonen (jku) 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.
Comment 8 Jussi Kukkonen (jku) 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.
Comment 9 Martin Robinson 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.
Comment 10 Michael Pruett 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.
Comment 11 Martin Robinson 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.
Comment 12 Martin Robinson 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.
Comment 13 Jussi Kukkonen (jku) 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.
Comment 14 David Grogan 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.
Comment 15 Martin Robinson 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?
Comment 16 David Grogan 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.
Comment 17 Jussi Kukkonen (jku) 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.
Comment 18 Jussi Kukkonen (jku) 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.
Comment 19 Martin Robinson 2013-01-10 05:17:39 PST
Great. I have a patch in my working copy which I expect to post today.
Comment 20 Martin Robinson 2013-01-10 10:57:58 PST
Created attachment 182169 [details]
Patch
Comment 21 Martin Robinson 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.
Comment 22 Martin Robinson 2013-01-17 09:07:03 PST
Created attachment 183197 [details]
Patch
Comment 23 Gustavo Noronha (kov) 2013-01-17 09:20:28 PST
Comment on attachment 183197 [details]
Patch

OK
Comment 24 WebKit Review Bot 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>
Comment 25 WebKit Review Bot 2013-01-17 12:04:20 PST
All reviewed patches have been landed.  Closing bug.