Bug 174253 – [CMake] Clean up Web Crypto build targets

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[CMake] Clean up Web Crypto build targets

[Zan Dobersek]

Created attachment 314843 [details]
Patch

[Michael Catanzaro]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Created attachment 315212 [details]
Patch

Also uses USE_GCRYPT guards, doesn't (yet) add a GCrypt.cmake file.

[Michael Catanzaro]

(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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Created attachment 315640 [details]
Patch

[Michael Catanzaro]

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.

[Zan Dobersek]

Comment on attachment 315640 [details]
Patch

Clearing flags on attachment: 315640

Committed r219556: <http://trac.webkit.org/changeset/219556>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Reverted r219556 for reason:

Broke build without WebCrypto

Committed r219559: <http://trac.webkit.org/changeset/219559>

[Michael Catanzaro]

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'

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 315916 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Michael Catanzaro]

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.

[Zan Dobersek]

Committed r219676: <http://trac.webkit.org/changeset/219676>