Bug 174253 - [CMake] Clean up Web Crypto build targets
Summary: [CMake] Clean up Web Crypto build targets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-07-07 07:05 PDT by Zan Dobersek
Modified: 2017-07-19 22:57 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.03 KB, patch)
2017-07-07 07:12 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (14.42 KB, patch)
2017-07-12 00:24 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (14.13 KB, patch)
2017-07-16 23:19 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (15.76 KB, patch)
2017-07-19 06:05 PDT, Zan Dobersek
achristensen: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.64 MB, application/zip)
2017-07-19 07:30 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-07-07 07:05:28 PDT
[CMake] Clean up Web Crypto build targets
Comment 1 Zan Dobersek 2017-07-07 07:12:38 PDT
Created attachment 314843 [details]
Patch
Comment 2 Michael Catanzaro 2017-07-07 08:59:50 PDT
Comment on attachment 314843 [details]
Patch

The pattern we've been following recently would be to create a GCrypt.cmake file with the GCrypt-specific targets. And guard the .cpp files with CMake conditionals so that they are not built at all when the feature is disabled.
Comment 3 Zan Dobersek 2017-07-12 00:09:06 PDT
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 314843 [details]
> Patch
> 
> The pattern we've been following recently would be to create a GCrypt.cmake
> file with the GCrypt-specific targets. And guard the .cpp files with CMake
> conditionals so that they are not built at all when the feature is disabled.

Where would the CMake file be placed? All such files are under platform/ for now, but it would be odd to add GCrypt.cmake there, and then from that file add files under crypto/ to the build.
Comment 4 Zan Dobersek 2017-07-12 00:24:20 PDT
Created attachment 315212 [details]
Patch

Also uses USE_GCRYPT guards, doesn't (yet) add a GCrypt.cmake file.
Comment 5 Michael Catanzaro 2017-07-12 08:13:21 PDT
(In reply to Zan Dobersek from comment #3)
> Where would the CMake file be placed? All such files are under platform/ for
> now,

That's exactly where I would put it.

> but it would be odd to add GCrypt.cmake there, and then from that file
> add files under crypto/ to the build.

I don't see why it would be odd.
Comment 6 Zan Dobersek 2017-07-16 22:54:38 PDT
(In reply to Michael Catanzaro from comment #5)
> (In reply to Zan Dobersek from comment #3)
> > Where would the CMake file be placed? All such files are under platform/ for
> > now,
> 
> That's exactly where I would put it.
> 
> > but it would be odd to add GCrypt.cmake there, and then from that file
> > add files under crypto/ to the build.
> 
> I don't see why it would be odd.

OK, I don't care enough about this.
Comment 7 Zan Dobersek 2017-07-16 23:19:11 PDT
Created attachment 315640 [details]
Patch
Comment 8 Michael Catanzaro 2017-07-16 23:49:51 PDT
Comment on attachment 315640 [details]
Patch

Exactly! Sony starting splitting some libs they use out to separate CMake files already... we'll probably move more.
Comment 9 Zan Dobersek 2017-07-17 01:45:57 PDT
Comment on attachment 315640 [details]
Patch

Clearing flags on attachment: 315640

Committed r219556: <http://trac.webkit.org/changeset/219556>
Comment 10 Zan Dobersek 2017-07-17 01:46:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Michael Catanzaro 2017-07-17 07:56:50 PDT
Reverted r219556 for reason:

Broke build without WebCrypto

Committed r219559: <http://trac.webkit.org/changeset/219559>
Comment 12 Michael Catanzaro 2017-07-17 07:58:25 PDT
We'll have to try again. It broke the build without WebCrypto. Some of the stuff that you moved behind ENABLE_SUBTLE_CRYTPO is needed for CryptoDigest:

lib/libPAL.a(lib/../Source/WebCore/PAL/pal/CMakeFiles/PAL.dir/crypto/gcrypt/CryptoDigestGCrypt.cpp.o):CryptoDigestGCrypt.cpp:function PAL::CryptoDigest::create(PAL::CryptoDigest::Algorithm): error: undefined reference to 'gcry_md_open'
lib/libPAL.a(lib/../Source/WebCore/PAL/pal/CMakeFiles/PAL.dir/crypto/gcrypt/CryptoDigestGCrypt.cpp.o):CryptoDigestGCrypt.cpp:function PAL::CryptoDigest::computeHash(): error: undefined reference to 'gcry_md_get_algo_dlen'
lib/libPAL.a(lib/../Source/WebCore/PAL/pal/CMakeFiles/PAL.dir/crypto/gcrypt/CryptoDigestGCrypt.cpp.o):CryptoDigestGCrypt.cpp:function PAL::CryptoDigest::computeHash(): error: undefined reference to 'gcry_md_ctl'
lib/libPAL.a(lib/../Source/WebCore/PAL/pal/CMakeFiles/PAL.dir/crypto/gcrypt/CryptoDigestGCrypt.cpp.o):CryptoDigestGCrypt.cpp:function PAL::CryptoDigest::computeHash(): error: undefined reference to 'gcry_md_read'
lib/libPAL.a(lib/../Source/WebCore/PAL/pal/CMakeFiles/PAL.dir/crypto/gcrypt/CryptoDigestGCrypt.cpp.o):CryptoDigestGCrypt.cpp:function PAL::CryptoDigest::computeHash(): error: undefined reference to 'gcry_md_close'
lib/libPAL.a(lib/../Source/WebCore/PAL/pal/CMakeFiles/PAL.dir/crypto/gcrypt/CryptoDigestGCrypt.cpp.o):CryptoDigestGCrypt.cpp:function PAL::CryptoDigest::addBytes(void const*, unsigned long): error: undefined reference to 'gcry_md_write'
Comment 13 Zan Dobersek 2017-07-19 05:57:42 PDT
USE_GCRYPT has to be set and exposed to the build for both GTK+ and WPE since it's used in the libgcrypt-based CryptoDigest implementation that's used by the two ports unconditionally.

The problem was libgcrypt include dirs and libraries weren't used during the build for the GTK+ port because that was done only in case USE_GCRYPT was enabled and GCrypt.cmake included in the build, but USE_GCRYPT wasn't enabled if ENABLE_SUBTLE_CRYPTO wasn't. The WPE port didn't suffer this because it added libgcrypt include dirs and libraries to the build for the needs of ClearKey support.
Comment 14 Zan Dobersek 2017-07-19 06:05:50 PDT
Created attachment 315916 [details]
Patch
Comment 15 Build Bot 2017-07-19 07:30:46 PDT
Comment on attachment 315916 [details]
Patch

Attachment 315916 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4147899

New failing tests:
fast/shadow-dom/ShadowRoot-mode.html
Comment 16 Build Bot 2017-07-19 07:30:48 PDT
Created attachment 315918 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 17 Michael Catanzaro 2017-07-19 11:40:13 PDT
Comment on attachment 315916 [details]
Patch

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

> Source/WebCore/PlatformGTK.cmake:13
> +if (USE_GCRYPT)
> +    include(platform/GCrypt.cmake)
> +endif ()

Since it's always true now, you should remove the conditional.

> Source/WebCore/PlatformWPE.cmake:10
> +if (USE_GCRYPT)
> +    include(platform/GCrypt.cmake)
> +endif ()

Ditto.
Comment 18 Zan Dobersek 2017-07-19 22:57:38 PDT
Committed r219676: <http://trac.webkit.org/changeset/219676>