Add CMake build files #60

yim-lee · 2021-01-20T19:40:02Z

Motivation:
This is to support swiftlang/swift-package-manager#3202, which adds swift-crypto as a dependency to SwiftPM.

Modifications:
Add CMake files.

Result:
Can build swift-crypto with cmake.

yim-lee · 2021-01-20T19:44:21Z

@compnerd @neonichu - please review

neonichu · 2021-01-20T19:51:28Z

Do we need to make sure we carry this define with us to keep Windows support? https://github.com/apple/swift-crypto/blob/main/Package.swift#L57 @compnerd

Lukasa · 2021-01-20T21:16:09Z

Do we have any plan to keep the CMakeLists.txt up-to-date?

yim-lee · 2021-01-20T23:00:54Z

Marking this a draft since there are build errors

[293/305] Building C object Sources/CCryptoBoringSSLShims/CMakeFiles/CCryptoBoringSSLShims.dir/shims.c.o
FAILED: Sources/CCryptoBoringSSLShims/CMakeFiles/CCryptoBoringSSLShims.dir/shims.c.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc  -ISources/CCryptoBoringSSLShims/include -Iswift -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -mmacosx-version-min=11.0 -MD -MT Sources/CCryptoBoringSSLShims/CMakeFiles/CCryptoBoringSSLShims.dir/shims.c.o -MF Sources/CCryptoBoringSSLShims/CMakeFiles/CCryptoBoringSSLShims.dir/shims.c.o.d -o Sources/CCryptoBoringSSLShims/CMakeFiles/CCryptoBoringSSLShims.dir/shims.c.o   -c Sources/CCryptoBoringSSLShims/shims.c
In file included from Sources/CCryptoBoringSSLShims/shims.c:14:
Sources/CCryptoBoringSSLShims/include/CCryptoBoringSSLShims.h:22:10: fatal error: 'CCryptoBoringSSL.h' file not found
#include <CCryptoBoringSSL.h>
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

I haven't figured out how to fix this yet.

Do we have any plan to keep the CMakeLists.txt up-to-date?

@Lukasa - we might be able to use env var instead of hardcoding the list of files but I could be wrong.

neonichu · 2021-01-20T23:28:12Z

Do we have any plan to keep the CMakeLists.txt up-to-date?

Maybe we can add building with CMake as a step to CI so that there would at least be feedback if a PR breaks it?

Lukasa · 2021-01-21T09:44:05Z

I think we will absolutely have to do @neonichu's suggestion, irregardless of anything else. If we don't add that step I guarantee that we'll break you down the road because I'll 100% forget to make sure this still works.

However, the other problem we have is that you're currently explicitly naming every single file in the CBoringSSL target. This target's files are updated as a monolithic block with some regularity, and the exact files in there have high churn. Is there some way we can express the list of files by way of a glob? If not, I think I'd like us to invest in some scripting to automatically generate the file list for a given target. This should be fairly easy, as right now we're just following the rules that SwiftPM lays out. Essentially I want some way to say "look at this Package.swift and generate the appropriate CMakeLists.txt equivalent". I don't think this is particularly technically challenging, but would drastically reduce the maintenance burden of the CMake build system.

Sources/CCryptoBoringSSLShims/CMakeLists.txt

Sources/CCryptoBoringSSL/CMakeLists.txt

compnerd · 2021-01-21T17:21:46Z

Sources/Crypto/CMakeLists.txt

+#
+# SPDX-License-Identifier: Apache-2.0
+
+add_library(Crypto


Should this be a static library or a shared library? At least on Windows, static linking doesn't work yet, and needs to be shared.

You mean this should say add_library(Crypto SHARED? The examples that I've looked at don't specify SHARED and only the C modules have STATIC so I don't know what's the right thing to do here.

Well, the current signature leaves it in the control of the user. If you specify -D BUILD_SHARED_LIBS=YES then this is implicitly a dynamic library and otherwise (or if you do -D BUILD_SHARED_LIBS=NO) it is implicitly a static library. The reason for not specifying an explicit shared/static here is to allow the user to select the build type. This is why setting the default value for BUILD_SHARED_LIBS to YES is what I suggested above.

CMakeLists.txt

Sources/CCryptoBoringSSL/CMakeLists.txt

compnerd · 2021-01-21T17:27:07Z

The problem with using a GLOB for the file list is that any changes will not be picked up. You will need to remember that you changed things and do a clean build. I think that generating the file-list should be possible and would be a much nicer approach to just update the list as part of the generation script (as the updates are scripted).

neonichu · 2021-01-21T19:37:45Z

I think we should be able to script this using swift package describe which will tell us the list of source files according to SwiftPM rules.

Sources/CCryptoBoringSSLShims/CMakeLists.txt

tomerd · 2021-01-22T05:49:59Z

I think we will absolutely have to do @neonichu's suggestion, irregardless of anything else. If we don't add that step I guarantee that we'll break you down the road because I'll 100% forget to make sure this still works.

lmk if and when we need to set a CI job for this

yim-lee · 2021-01-22T07:10:04Z

@swift-server-bot test this please

yim-lee · 2021-01-22T07:15:09Z

Not sure how 5f80571 causes these errors.

https://ci.swiftserver.group/job/swift-crypto-api-breakage-prb/68/console

23:11:03 [337/384] Compiling Crypto EllipticCurvePoint_boring.swift
23:11:03 <unknown>:0: warning: missing submodule 'Dispatch'
23:11:03 <unknown>:0: error: cannot load underlying module for 'Dispatch'
23:11:04 [338/384] Compiling Crypto AES-GCM.swift
23:11:04 <unknown>:0: warning: missing submodule 'CCryptoBoringSSL'
23:11:04 /tmp/.check-api_Kwg0eL/repo/Sources/Crypto/AEADs/AES/GCM/BoringSSL/AES-GCM_boring.swift:17:29: error: no such module 'CCryptoBoringSSL'
23:11:04 @_implementationOnly import CCryptoBoringSSL

https://ci.swiftserver.group/job/swift-crypto-api-breakage-prb/69/console

23:16:07 <unknown>:0: error: module file '/tmp/.check-api_Ig7NGS/repo/.build/x86_64-unknown-linux/debug/ModuleCache/K9FEF4G1JFDL/_Builtin_stddef_max_align_t-1I501HZAI4NHE.pcm' is out of date and needs to be rebuilt: signature mismatch
23:16:07 <unknown>:0: note: imported by module 'CoreFoundation' in '/tmp/.check-api_Ig7NGS/repo/.build/x86_64-unknown-linux/debug/ModuleCache/K9FEF4G1JFDL/CoreFoundation-3D1XS5ROKRWPI.pcm'
23:16:07 swift: /home/buildnode/jenkins/workspace/oss-swift-5.1-package-linux-ubuntu-18_04/llvm/tools/clang/lib/Serialization/InMemoryModuleCache.cpp:36: llvm::MemoryBuffer &clang::InMemoryModuleCache::addBuiltPCM(llvm::StringRef, std::unique_ptr<llvm::MemoryBuffer>): Assertion `!PCM.IsFinal && "Trying to override finalized PCM?"' failed.

@swift-server-bot test this please

yim-lee · 2021-01-22T07:51:01Z

@swift-server-bot test this please

CMakeLists.txt

yim-lee · 2021-01-23T00:09:50Z

"soundess" validation failure is expected until #63 gets merged

yim-lee · 2021-01-23T00:13:42Z

This is ready for review.

.gitignore

compnerd · 2021-01-24T19:42:20Z

Sources/CCryptoBoringSSL/CMakeLists.txt

+      crypto/fipsmodule/x86_64-mont.mac.x86_64.S
+      crypto/fipsmodule/x86_64-mont5.mac.x86_64.S)
+  endif()
+endif()


We should cover windows and Linux as well, but that is okay to have as a follow up.

Which ones are for Windows? I don't see any *x86_64* for it.

Windows has some assembly routines for x86, not for x64. I don't think that it matters too much if we don't get that variant correct. I don't currently intend to ship a 32-bit toolchain. For Windows x86, use of swift-crypto as a client should go through SPM, which will handle it properly. If someone intends to ship a non-x64 variant of the toolchain, we should handle it at that time.

OK. Added Linux 6d2e5c1

scripts/check_no_api_breakages.sh

yim-lee · 2021-01-25T00:43:45Z

I added scripts/update_cmakelists.txt (beac548) that finds source files under a directory then updates that specific part of the corresponding CMakeLists.txt. Hopefully that makes things easier?

yim-lee · 2021-01-26T07:25:46Z

Build error unrelated. Looks like build agent 1 is busted.

@swift-server-bot test this please

Motivation: This is to support swiftlang/swift-package-manager#3202, which adds swift-crypto as a dependency to SwiftPM. Modifications: Add CMake files. Result: Can build swift-crypto with cmake.

Make the BoringSSL based build for macOS complete enough. We can now build a shared library version of SwiftCrypto on macOS without CryptoKit.

Script was failing at "build_and_do" steps. Order seems to matter. - When build new then old, build fails. - When build old then new, build succeeds. This might be because "new" build adds files to `.build` that "old" doesn't expect? Probably because of the `.modulemap` files that were added as part of the PR. Cleaning up `.build` in between builds seems to fix the problem.

Regenerate CMakeLists.txt using script

yim-lee · 2021-01-26T20:26:30Z

Updated soundness.sh to check for source file changes that would require updating CMakeLists.txt, in which case CI would fail and prevent merge (for non-admin at least).

yim-lee · 2021-01-26T20:27:23Z

@tomerd build agent for 5.3 is still no good: https://ci.swiftserver.group/job/swift-crypto-swift53-prb/59/console

tomerd · 2021-01-26T20:58:09Z

@swift-server-bot test this please

compnerd · 2021-01-26T23:44:42Z

LGTM FWIW

Lukasa · 2021-01-27T13:57:00Z

scripts/update_cmakelists.sh

+
+update_cmakelists "CCryptoBoringSSL" "*.c"
+update_cmakelists "CCryptoBoringSSLShims" "*.c"
+update_cmakelists "Crypto" "*.swift"


Are we not generating the .S list on purpose?

Just wasn't sure if they change often. Sounds like we should generate?

If we can, yeah. They do change with some frequency.

Amended 4a1b864

Lukasa

Ok, I think this will work. Thanks for making it something we can maintain going forward @yim-lee, and thanks for your help along the way @compnerd and @neonichu.

Motivation: SwiftPM's CMake build (swiftlang/swift-package-manager#3202) unsuccessful even with apple#60. Modifications: - Use consistent `SWIFT_CRYPTO_EXPORTS` naming. In some places `CRYPTO_EXPORTS` was used instead. - Install `CCryptoBoringSSL` so we can import it in code (see swiftlang/swift-package-manager#3202) - Always set `BUILD_SHARED_LIBS` Result: Successful CMake build.

Motivation: SwiftPM's CMake build (swiftlang/swift-package-manager#3202) unsuccessful even with #60. Modifications: - Use consistent `SWIFT_CRYPTO_EXPORTS` naming. In some places `CRYPTO_EXPORTS` was used instead. - Install `CCryptoBoringSSL` so we can import it in code (see swiftlang/swift-package-manager#3202) - Always set `BUILD_SHARED_LIBS` Result: Successful CMake build.

yim-lee requested review from Lukasa and tomerd January 20, 2021 19:43

yim-lee changed the title ~~Add CMake build files~~ [Draft] Add CMake build files Jan 20, 2021

compnerd reviewed Jan 21, 2021

View reviewed changes

yim-lee commented Jan 22, 2021

View reviewed changes

Sources/CCryptoBoringSSLShims/CMakeLists.txt Show resolved Hide resolved

yim-lee mentioned this pull request Jan 22, 2021

CMake build support #62

Closed

5 tasks

compnerd reviewed Jan 22, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

yim-lee force-pushed the cmake branch from bfcd792 to 2cead58 Compare January 23, 2021 00:09

yim-lee changed the title ~~[Draft] Add CMake build files~~ Add CMake build files Jan 23, 2021

compnerd reviewed Jan 24, 2021

View reviewed changes

yim-lee force-pushed the cmake branch from 50b9ce5 to beac548 Compare January 25, 2021 00:39

yim-lee force-pushed the cmake branch from 6d2e5c1 to a50c95d Compare January 26, 2021 17:41

yim-lee added 2 commits January 26, 2021 11:33

Add CMake build files

16cd664

Motivation: This is to support swiftlang/swift-package-manager#3202, which adds swift-crypto as a dependency to SwiftPM. Modifications: Add CMake files. Result: Can build swift-crypto with cmake.

Specify list of files for CCryptoBoringSSL

84c5d33

yim-lee and others added 13 commits January 26, 2021 11:33

Builds on ubuntu-focal

5aa1ff2

Trying finding and linking CryptoKit on Apple platforms

b3fa911

Set '-fPIC' the correct way

69512b9

build: build macOS against BoringSSL

56c512e

Make the BoringSSL based build for macOS complete enough. We can now build a shared library version of SwiftCrypto on macOS without CryptoKit.

Script to update CMakeLists.txt

2e0bae2

Regenerate CMakeLists.txt using script

Update gitignore per Saleem's feedback

d8a1295

Add code comment

146f23b

Improved 'find'

f73947a

Add assembly files for Apple and Linux

2f3091f

Add cmake files to license header check

5b2e9c6

Add check for source file changes that would impact CMakeLists.txt

064a2cb

yim-lee force-pushed the cmake branch from 8db27a0 to 064a2cb Compare January 26, 2021 19:33

Make sort consistent across platforms

9093150

Lukasa reviewed Jan 27, 2021

View reviewed changes

yim-lee added 2 commits January 27, 2021 09:45

Missing 'CMAKE_SYSTEM_PROCESSOR MATCHES' check

ace3765

Generate .S list

4a1b864

yim-lee force-pushed the cmake branch from 937bdd3 to 4a1b864 Compare January 27, 2021 18:32

Lukasa approved these changes Jan 27, 2021

View reviewed changes

Lukasa merged commit 47640ad into apple:main Jan 27, 2021

yim-lee deleted the cmake branch January 27, 2021 20:38

yim-lee mentioned this pull request Jan 28, 2021

Update/Fix CMake build #65

Merged

Add CMake build files #60

Add CMake build files #60

Uh oh!

Conversation

yim-lee commented Jan 20, 2021

Uh oh!

yim-lee commented Jan 20, 2021

Uh oh!

neonichu commented Jan 20, 2021

Uh oh!

Lukasa commented Jan 20, 2021

Uh oh!

yim-lee commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonichu commented Jan 20, 2021

Uh oh!

Lukasa commented Jan 21, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

compnerd commented Jan 21, 2021

Uh oh!

neonichu commented Jan 21, 2021

Uh oh!

Uh oh!

tomerd commented Jan 22, 2021

Uh oh!

yim-lee commented Jan 22, 2021

Uh oh!

yim-lee commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yim-lee commented Jan 22, 2021

Uh oh!

Uh oh!

yim-lee commented Jan 23, 2021

Uh oh!

yim-lee commented Jan 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yim-lee commented Jan 25, 2021

Uh oh!

yim-lee commented Jan 26, 2021

Uh oh!

yim-lee commented Jan 26, 2021

Uh oh!

yim-lee commented Jan 26, 2021

Uh oh!

tomerd commented Jan 26, 2021

Uh oh!

compnerd commented Jan 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

yim-lee commented Jan 20, 2021 •

edited

Loading

yim-lee Jan 22, 2021 •

edited

Loading

yim-lee commented Jan 22, 2021 •

edited

Loading

yim-lee Jan 27, 2021 •

edited

Loading