Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add support for S2N_INTERN_LIBCRYPTO with FetchContent #5076

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

kou
Copy link
Contributor

@kou kou commented Feb 3, 2025

Release Summary:

Adds support for consuming s2n-tls from CMake FetchContent with interning enabled.

Resolved issues:

resolves #5075.

Description of changes:

lib/libs2n.a doesn't exist in ${CMAKE_CURRENT_BINARY_DIR} with FetchContent. So

s2n-tls/CMakeLists.txt

Lines 446 to 452 in c6b41ef

add_custom_command(
TARGET ${PROJECT_NAME} POST_BUILD
DEPENDS libcrypto.symbols
COMMAND
bash -c "${CMAKE_AR} -r lib/libs2n.a s2n_libcrypto/*.o"
VERBATIM
)
failed with /bin/ar: lib/libs2n.a: No such file or directory.

We can use $<TARGET_FILE:${PROJECT_NAME}> instead of lib/libs2n.a to get the real libs2n.a path with/without FetchContent.

Call-outs:

Is there code added that needs to be cleaned up later?

No.

Is there code that is missing because it’s still in development?

No.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)?

We can use the reproducible steps in #5075. We can use this branch by the following CMakeLists.txt:

cmake_minimum_required(VERSION 3.25)

project(s2n-tls-fetchcontent)

include(FetchContent)

fetchcontent_declare(s2n-tls
                     OVERRIDE_FIND_PACKAGE
                     GIT_REPOSITORY https://github.com/kou/s2n-tls.git
                     GIT_TAG intern-fetchcontent)
set(S2N_INTERN_LIBCRYPTO ON CACHE BOOL "" FORCE)
fetchcontent_makeavailable(s2n-tls)

What manual testing was performed?

cmake -S . -B build -GNinja
ninja -C build lib/libs2n.a

They succeeded. See also the attached full logs:

Are there any testing steps to be verified by the reviewer?

The reviewer can use the same steps.

How can you convince your reviewers that this PR is safe and effective?

$<TARGET_FILE:XXX> is a standard CMake feature and a portable way to get .a file path. See also the CMake document: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:TARGET_FILE

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

No.

Remember:

  • Any change to the library source code should at least include unit tests.
  • Any change to the core stuffer or blob methods should include CBMC proofs.
  • Any change to the CI or tests should:
    1. prove that the test succeeds for good input
    2. prove that the test fails for bad input (eg, a test for memory leaks fails when a memory leak is committed)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

`lib/libs2n.a` doesn't exist in `${CMAKE_CURRENT_BINARY_DIR}` with
FetchContent. We can use `$<TARGET_FILE:${PROJECT_NAME}>` to get the
real `libs2n.a` path with/without FetchContent.
Copy link
Contributor

@goatgoose goatgoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I opened an issue for us to make sure we continue to support FetchContent: #5079

@goatgoose goatgoose requested a review from lrstewart February 4, 2025 16:46
@goatgoose goatgoose added this pull request to the merge queue Feb 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2025
@kou
Copy link
Contributor Author

kou commented Feb 5, 2025

Thanks.

It seems that the merge queue CI failed with a network error:

https://github.com/aws/s2n-tls/actions/runs/13144719007/job/36680080693#step:11:37

 ---- network::tls_client::kms_pq::pq_handshake stdout ----
2025-02-04T21:11:57.707088Z  INFO integration::network::tls_client: querying kms.us-east-1.amazonaws.com with Policy("PQ-TLS-1-2-2023-10-09")
thread 'network::tls_client::kms_pq::pq_handshake' panicked at integration/src/network/tls_client.rs:56:9:
assertion `left == right` failed
  left: Some("SecP256r1Kyber768Draft00")
 right: Some("x25519_kyber-512-r3")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    network::tls_client::kms_pq::pq_handshake

Could you retry it?

@goatgoose goatgoose added this pull request to the merge queue Feb 5, 2025
Merged via the queue into aws:main with commit b6932c5 Feb 5, 2025
45 checks passed
@kou kou deleted the intern-fetchcontent branch February 6, 2025 01:19
@kou kou restored the intern-fetchcontent branch February 6, 2025 02:46
@kou kou deleted the intern-fetchcontent branch February 6, 2025 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S2N_INTERN_LIBCRYPTO=ON doesn't work with FetchContent
3 participants