RESOLVED FIXED 106443
[CMake][EFL] Build ThirdParty/leveldb when IndexedDB is enabled 
https://bugs.webkit.org/show_bug.cgi?id=106443
Summary [CMake][EFL] Build ThirdParty/leveldb when IndexedDB is enabled
Jussi Kukkonen (jku)
Reported 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.
Attachments
Patch (3.93 KB, patch)
2013-01-09 05:36 PST, Jussi Kukkonen (jku) 		no flags
DetailsFormatted DiffDiff
Patch (5.49 KB, patch)
2013-01-18 03:54 PST, Jussi Kukkonen (jku) 		no flags
DetailsFormatted DiffDiff
ImImplement changes suggested by mr Kubo (5.41 KB, patch)
2013-01-18 05:21 PST, Jussi Kukkonen (jku) 		no flags
DetailsFormatted DiffDiff
Use WEBKIT_SET_EXTRA_COMPILER_FLAGS (5.40 KB, patch)
2013-01-24 04:30 PST, Jussi Kukkonen (jku) 		no flags
DetailsFormatted DiffDiff
Add IGNORECXX_WARNINGS as Laszlo suggests (5.41 KB, patch)
2013-01-24 09:34 PST, Jussi Kukkonen (jku) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Jussi Kukkonen (jku)
Comment 1 2013-01-09 05:36:51 PST
Created attachment 181901 [details] Patch
Jussi Kukkonen (jku)
Comment 2 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
Jussi Kukkonen (jku)
Comment 3 2013-01-18 03:54:44 PST
Created attachment 183422 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 4 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.
Jussi Kukkonen (jku)
Comment 5 2013-01-18 05:21:05 PST
Created attachment 183428 [details] ImImplement changes suggested by mr Kubo
Jussi Kukkonen (jku)
Comment 6 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?
Laszlo Gombos
Comment 7 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 ?
Jussi Kukkonen (jku)
Comment 8 2013-01-24 04:30:38 PST
Created attachment 184462 [details] Use WEBKIT_SET_EXTRA_COMPILER_FLAGS
Jussi Kukkonen (jku)
Comment 9 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.
Laszlo Gombos
Comment 10 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 ?
Jussi Kukkonen (jku)
Comment 11 2013-01-24 09:34:01 PST
Created attachment 184516 [details] Add IGNORECXX_WARNINGS as Laszlo suggests
Laszlo Gombos
Comment 12 2013-01-24 09:38:34 PST
Comment on attachment 184516 [details] Add IGNORECXX_WARNINGS as Laszlo suggests r=me.
Gyuyoung Kim
Comment 13 2013-01-24 23:58:21 PST
Comment on attachment 184516 [details] Add IGNORECXX_WARNINGS as Laszlo suggests Looks fine as well.
WebKit Review Bot
Comment 14 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>
WebKit Review Bot
Comment 15 2013-01-25 02:19:42 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.