Bug 106443 - [CMake][EFL] Build ThirdParty/leveldb when IndexedDB is enabled
Summary: [CMake][EFL] Build ThirdParty/leveldb when IndexedDB is enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jussi Kukkonen (jku)
URL:
Keywords:
Depends on: 103220 107247
Blocks: 107248
  Show dependency treegraph
 
Reported: 2013-01-09 04:50 PST by Jussi Kukkonen (jku)
Modified: 2013-01-25 02:19 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.93 KB, patch)
2013-01-09 05:36 PST, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2013-01-18 03:54 PST, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
ImImplement changes suggested by mr Kubo (5.41 KB, patch)
2013-01-18 05:21 PST, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Use WEBKIT_SET_EXTRA_COMPILER_FLAGS (5.40 KB, patch)
2013-01-24 04:30 PST, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Add IGNORECXX_WARNINGS as Laszlo suggests (5.41 KB, patch)
2013-01-24 09:34 PST, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jussi Kukkonen (jku) 2013-01-09 04:50:35 PST
We'll need to copypasta at least part of leveldb (see bug 103220), but there are also build fixes needed.

I'll attach a patch, but let's not commit it until there's a decision on 103220.
Comment 1 Jussi Kukkonen (jku) 2013-01-09 05:36:51 PST
Created attachment 181901 [details]
Patch
Comment 2 Jussi Kukkonen (jku) 2013-01-18 00:40:08 PST
Comment on attachment 181901 [details]
Patch

Martin ended up adding leveldb to ThirdParty/. I'll redo this patch
Comment 3 Jussi Kukkonen (jku) 2013-01-18 03:54:44 PST
Created attachment 183422 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2013-01-18 04:06:57 PST
Comment on attachment 183422 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2666
> +        "${THIRDPARTY_DIR}/leveldb/"

Minor nitpick: the trailing slash is probably unneeded.

> Source/WebCore/CMakeLists.txt:3066
> +    set(LEVELDB_LIBRARY_NAME LEVELDB)

While technically OK, I think lowercasing the library name makes it more consistent (ie. having it called libleveldb.a instead of libLEVELDB.a).

> Source/WebCore/CMakeLists.txt:3069
> +    set_target_properties(${LEVELDB_LIBRARY_NAME} PROPERTIES FOLDER "WebCore")

This is probably unneeded; these days we put all generated libraries in the same directory by setting the `CMAKE_LIBRARY_OUTPUT_DIRECTORY' variable in the top-level CMakeLists.txt; the fact that we still have calls like these hanging around in other files is a relic from the past.
Comment 5 Jussi Kukkonen (jku) 2013-01-18 05:21:05 PST
Created attachment 183428 [details]
ImImplement changes suggested by mr Kubo
Comment 6 Jussi Kukkonen (jku) 2013-01-18 05:24:14 PST
Comment on attachment 183428 [details]
ImImplement changes suggested by mr Kubo

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

> Source/WebCore/CMakeLists.txt:3070
> +    set_target_properties(${LEVELDB_LIBRARY_NAME} PROPERTIES COMPILE_FLAGS "-fno-builtin-memcmp -fPIC")

The '-fPIC' I'm not totally sure about... It is needed to build with efl wk2 but that's probably because of SHARED_CORE, right? Does this need to be conditional in some way?
Comment 7 Laszlo Gombos 2013-01-21 19:55:58 PST
(In reply to comment #6)
> (From update of attachment 183428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183428&action=review
> 
> > Source/WebCore/CMakeLists.txt:3070
> > +    set_target_properties(${LEVELDB_LIBRARY_NAME} PROPERTIES COMPILE_FLAGS "-fno-builtin-memcmp -fPIC")
> 
> The '-fPIC' I'm not totally sure about... It is needed to build with efl wk2 but that's probably because of SHARED_CORE, right? Does this need to be conditional in some way?

Can we use the WEBKIT_SET_EXTRA_COMPILER_FLAGS macro instead to set fPIC (and whatever else is set in the macro) just like we do for the ANGLE library ?
Comment 8 Jussi Kukkonen (jku) 2013-01-24 04:30:38 PST
Created attachment 184462 [details]
Use WEBKIT_SET_EXTRA_COMPILER_FLAGS
Comment 9 Jussi Kukkonen (jku) 2013-01-24 04:33:04 PST
Latest patch uses WEBKIT_SET_EXTRA_COMPILER_FLAGS as Laszlo suggested. This automatically sets -fPIC when needed (among other things). I've dropped no-builtin-memcmp for now, let's add that if the speed really is a problem.
Comment 10 Laszlo Gombos 2013-01-24 07:27:50 PST
Comment on attachment 184462 [details]
Use WEBKIT_SET_EXTRA_COMPILER_FLAGS

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

> Source/WebCore/CMakeLists.txt:3075
> +    WEBKIT_SET_EXTRA_COMPILER_FLAGS(${LEVELDB_LIBRARY_NAME})

We should disable warnings for LevelDB as those should be addressed upstream for LevelDB and not in the WebKit tree. Can we use
WEBKIT_SET_EXTRA_COMPILER_FLAGS(${LEVELDB_LIBRARY_NAME} IGNORECXX_WARNINGS)
instead ?
Comment 11 Jussi Kukkonen (jku) 2013-01-24 09:34:01 PST
Created attachment 184516 [details]
Add IGNORECXX_WARNINGS as Laszlo suggests
Comment 12 Laszlo Gombos 2013-01-24 09:38:34 PST
Comment on attachment 184516 [details]
Add IGNORECXX_WARNINGS as Laszlo suggests

r=me.
Comment 13 Gyuyoung Kim 2013-01-24 23:58:21 PST
Comment on attachment 184516 [details]
Add IGNORECXX_WARNINGS as Laszlo suggests

Looks fine as well.
Comment 14 WebKit Review Bot 2013-01-25 02:19:37 PST
Comment on attachment 184516 [details]
Add IGNORECXX_WARNINGS as Laszlo suggests

Clearing flags on attachment: 184516

Committed r140804: <http://trac.webkit.org/changeset/140804>
Comment 15 WebKit Review Bot 2013-01-25 02:19:42 PST
All reviewed patches have been landed.  Closing bug.