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

CI: Amalgamation Linkage Test and Fixes #4307

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

FAlbertDev
Copy link
Collaborator

@FAlbertDev FAlbertDev commented Aug 15, 2024

Warnings like the one in #4303 or #4291 (comment) are currently not checked by our CI. This is because we do not test including Botan headers while the BOTAN_IS_BEING_BUILT macro is undefined. The only target that is compiled without this flag is examples.
To fix this shortcoming, I propose adding the examples target to the amalgamation CI build and adding an example that includes Botan's amalgamation header. This has the following advantages:

  • Warnings in headers that depend on BOTAN_IS_BEING_BUILT are caught by CI (like the one in PR 4303)
  • We can test the examples target on different operating systems.
  • Since the amalgamation builds are quite fast compared to the normal ones, the additional overhead by adding boost capabilities and building the examples is not too painful.

As you can see in this experimental CI run, the issue of #4303 is found with this setup (seems to be an exclusive MSVC "feature"). Also, I found a few other issues in the examples when using macOS. Since these are related to TLS, @reneme, could you check and verify my proposed fixes?

Here are the errors for the fixed code sections:

src/examples/tls_ssl_key_log_file.cpp:

CI log:

ccache clang++ -fstack-protector -m64 -pthread -stdlib=libc++ -std=c++20 -D_REENTRANT  -O3 --system-header-prefix=boost/ -Wall -Wextra -Wpedantic -Wshadow -Wstrict-aliasing -Wstrict-overflow=5 -Wcast-align -Wmissing-declarations -Wpointer-arith -Wcast-qual -Wshorten-64-to-32 -Wcomma -Wdocumentation -Werror -Wno-error=unused-parameter -Wno-error=unreachable-code -Wno-error=unused-lambda-capture  -I build/include/public  -isystem build/include/external -isystem /usr/local/opt/boost/include -c src/examples/x509_path.cpp -o build/obj/examples/x509_path.o
src/examples/tls_ssl_key_log_file.cpp:204:15: error: no type named 'close' in the global namespace
            ::close(fd);
            ~~^
src/examples/tls_ssl_key_log_file.cpp:204:21: error: declaration shadows a field of '(anonymous namespace)::DtlsConnection' [-Werror,-Wshadow]
            ::close(fd);
                    ^
src/examples/tls_ssl_key_log_file.cpp:132:11: note: previous declaration is here
      int fd;
          ^
2 errors generated.
make: *** [build/obj/examples/tls_ssl_key_log_file.o] Error 1
src/examples/tls_stream_coroutine_client.cpp:

CI log:

ccache clang++ -fstack-protector -pthread -stdlib=libc++ build/obj/examples/tls_stream_coroutine_client.o -L.  -O3 --system-header-prefix=boost/ -lbotan-3 -lbz2 -ldl -lsqlite3 -lz -framework CoreFoundation -framework Security   -o build/examples/tls_stream_coroutine_client
Undefined symbols for architecture arm64:
  "Botan::TLS::Context::Context(Botan::TLS::Server_Information)", referenced from:
      void std::__1::allocator_traits<std::__1::allocator<Botan::TLS::Context>>::construct[abi:ue170006]<Botan::TLS::Context, Botan::TLS::Server_Information, void, void>(std::__1::allocator<Botan::TLS::Context>&, Botan::TLS::Context*, Botan::TLS::Server_Information&&) in tls_stream_coroutine_client.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build/examples/tls_stream_coroutine_client] Error 1
make: Target `examples' not remade because of errors.

@FAlbertDev FAlbertDev requested a review from reneme August 15, 2024 09:12
@coveralls
Copy link

coveralls commented Aug 15, 2024

Coverage Status

coverage: 91.118% (-0.002%) from 91.12%
when pulling 120398b on Rohde-Schwarz:ci/amalgamation-example
into 05f4053 on randombit:master.

@reneme
Copy link
Collaborator

reneme commented Aug 15, 2024

Since the amalgamation builds are quite fast compared to the normal ones, the additional overhead by adding boost capabilities and building the examples is not too painful.

Let me add that there's value in building the amalgamation with boost enabled in and by itself. There's a good chance that we break the amalgamation in headers that require boost (e.g. the TLS adapter).

src/examples/tls_ssl_key_log_file.cpp Outdated Show resolved Hide resolved
src/examples/tls_ssl_key_log_file.cpp Outdated Show resolved Hide resolved
src/examples/tls_stream_coroutine_client.cpp Outdated Show resolved Hide resolved
src/examples/tls_stream_coroutine_client.cpp Outdated Show resolved Hide resolved
src/examples/tls_stream_coroutine_client.cpp Outdated Show resolved Hide resolved
src/examples/amalgamation_header.cpp Show resolved Hide resolved
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

Nice! Thanks. A bit sad about the compiler pleasing in the examples, as it is potentially also incorporated into our handbook. But the alternative would be to lower the warning-levels for examples, which isn't desirable either, I guess? @randombit

src/examples/tls_ssl_key_log_file.cpp Outdated Show resolved Hide resolved
src/examples/tls_ssl_key_log_file.cpp Outdated Show resolved Hide resolved
@FAlbertDev FAlbertDev merged commit 7e02c5c into randombit:master Oct 14, 2024
37 checks passed
@FAlbertDev FAlbertDev deleted the ci/amalgamation-example branch October 14, 2024 08:29
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.

4 participants