Bug 175803 - [CMake] Use find_package for SQLite
Summary: [CMake] Use find_package for SQLite
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-21 17:50 PDT by Don Olmstead
Modified: 2017-08-24 19:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.32 KB, patch)
2017-08-21 17:51 PDT, Don Olmstead
achristensen: review+
Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2017-08-23 12:05 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2017-08-23 12:06 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (7.41 KB, patch)
2017-08-23 14:36 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2017-08-21 17:50:09 PDT
Should use find_package for SQLite installations.
Comment 1 Don Olmstead 2017-08-21 17:51:50 PDT
Created attachment 318712 [details]
Patch
Comment 2 Alex Christensen 2017-08-21 17:53:42 PDT
Comment on attachment 318712 [details]
Patch

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

> Source/WebKitLegacy/PlatformWin.cmake:-6
> -        "${WEBKIT_LIBRARIES_DIR}/include/sqlite"

AppleWin needs sqlite, too, I think.
Comment 3 Don Olmstead 2017-08-21 17:58:29 PDT
Comment on attachment 318712 [details]
Patch

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

>> Source/WebKitLegacy/PlatformWin.cmake:-6
>> -        "${WEBKIT_LIBRARIES_DIR}/include/sqlite"
> 
> AppleWin needs sqlite, too, I think.

Its in a if (${WTF_PLATFORM_WIN_CAIRO}) block.

I can have both AppleWin and WinCairo use find_package. Wasn't entirely sure if it would behave properly with SQLite3${DEBUG_SUFFIX}.
Comment 4 Alex Christensen 2017-08-21 18:01:44 PDT
Comment on attachment 318712 [details]
Patch

Ah, you're right.  This is fine.
Comment 5 Konstantin Tokarev 2017-08-22 02:42:43 PDT
Comment on attachment 318712 [details]
Patch

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

> Source/WebCore/PlatformWinCairo.cmake:7
> +    ${SQLITE_INCLUDE_DIR}

I think it should be in WebCore/CMakeLists.txt, as all ports use sqlite
Comment 6 Alex Christensen 2017-08-22 11:08:54 PDT
All ports use sqlite, but AppleWin needs the debug suffix in the internal debug build.
Comment 7 Don Olmstead 2017-08-23 12:05:23 PDT
Created attachment 318895 [details]
Patch
Comment 8 Don Olmstead 2017-08-23 12:06:15 PDT
Created attachment 318896 [details]
Patch
Comment 9 Konstantin Tokarev 2017-08-23 12:15:48 PDT
Comment on attachment 318896 [details]
Patch

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

> Source/WebKitLegacy/PlatformWin.cmake:-29
> -        PRIVATE SQLite3${DEBUG_SUFFIX}

Does find_package(Sqlite) handle debug suffix correctly? If no, it makes sense to use just

 set(SQLITE_LIBRARIES SQLite3${DEBUG_SUFFIX})

here
Comment 10 Don Olmstead 2017-08-23 13:00:49 PDT
(In reply to Konstantin Tokarev from comment #9)
> Comment on attachment 318896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318896&action=review
> 
> > Source/WebKitLegacy/PlatformWin.cmake:-29
> > -        PRIVATE SQLite3${DEBUG_SUFFIX}
> 
> Does find_package(Sqlite) handle debug suffix correctly? If no, it makes
> sense to use just
> 
>  set(SQLITE_LIBRARIES SQLite3${DEBUG_SUFFIX})
> 
> here

I'm looking into that. I'm not sure why the bot failed as I tried it locally and it finds SQLite3. There is no suffix in the release.
Comment 11 Don Olmstead 2017-08-23 14:36:01 PDT
Created attachment 318921 [details]
Patch
Comment 12 Don Olmstead 2017-08-23 15:33:46 PDT
Comment on attachment 318921 [details]
Patch

Changed this so its now for all ports.

The whole SQLite3${DEBUG_SUFFIX} seems to throw a wrench in using find_package for AppleWin so set(SQLITE_LIBRARIES) was used instead. ${SQLITE_INCLUDE_DIR} doesn't need to be set since its ignored when its just empty. If find_package were used without REQUIRED then it would be set to NOTFOUND which would error.

I think this is probably what we want to do with the other WebCore libraries that are common between all ports, zlib, xml2, xslt.
Comment 13 Konstantin Tokarev 2017-08-24 05:25:36 PDT
>The whole SQLite3${DEBUG_SUFFIX} seems to throw a wrench in using find_package for AppleWin

Not surpising actually, as FindSqlite.cmake makes no effort to find suffixed library
Comment 14 WebKit Commit Bot 2017-08-24 07:36:35 PDT
Comment on attachment 318921 [details]
Patch

Clearing flags on attachment: 318921

Committed r221135: <http://trac.webkit.org/changeset/221135>
Comment 15 WebKit Commit Bot 2017-08-24 07:36:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-08-24 07:37:05 PDT
<rdar://problem/34057821>
Comment 17 Csaba Osztrogonác 2017-08-24 08:57:04 PDT
(In reply to WebKit Commit Bot from comment #14)
> Comment on attachment 318921 [details]
> Patch
> 
> Clearing flags on attachment: 318921
> 
> Committed r221135: <http://trac.webkit.org/changeset/221135>

It broke the WinCairo build:
https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/5504
Comment 18 Don Olmstead 2017-08-24 19:34:43 PDT
(In reply to Csaba Osztrogonác from comment #17)
> (In reply to WebKit Commit Bot from comment #14)
> > Comment on attachment 318921 [details]
> > Patch
> > 
> > Clearing flags on attachment: 318921
> > 
> > Committed r221135: <http://trac.webkit.org/changeset/221135>
> 
> It broke the WinCairo build:
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/5504

I'm just going to rejigger the directory structure in WinCairoRequirements so it hopefully likes things better, https://bugs.webkit.org/show_bug.cgi?id=175972