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

Small CMake related tweaks #532

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Small CMake related tweaks #532

wants to merge 3 commits into from

Conversation

cpu
Copy link
Member

@cpu cpu commented Jan 20, 2025

add doc header to CMakeLists.txt

At least one person convinced themselves CMake is the required/preferred way to build librustls despite the README documenting the opposite. Let's try to be even more overt about it by adding a header to the root CMakeLists.txt.

In the near future I'd like to lift all of the CMake + test C code into a separate crate for even clearer separation.

rm CI dep on rustls-ffi-test for artifact test

Previously the artifacts.yaml CI job relied on an external repo that had a simple C/CMake application depending on librustls. When verifying artifacts we cloned this repo and tested a build against the artifact.

This commit reworks the main repo's test application code/build to support this use-case. The main tweak required is to support not building librustls from src, but instead finding it with pkg-config. We gate this behind a new FORCE_SYSTEM_RUSTLS option. The artifact test CI is updated to use this option. We want to leave the old default because it's the most convenient for developers of librustls: you want to hack on the Rust code and have your C test code builds pick up the changes without extra ceremony.

Annoyingly for the pkg-config workflow with the release artifacts zips (not the deb) we have to patch the .pc prefix to an absolute dir to get this working nicely, which means handling Windows specially.

Similarly, for yet to be determined reasons, on Win32 we have to add some extra target_link_libraries that don't propagate automatically through the .pc config (would be nice to sort this out one day). For now we just shift the existing Win32 specific target_link_libraries invocation up and use it for both pkg-config librustls and build-it-ourselves librustls.

Resolves #519

abort on CMake install w/ clear error

This updates the librustls CMakeLists.txt to abort on install. It's only intended to be used for the C examples, which we don't want anyone to install anywhere!

Use cargo capi for librustls, not cmake.

$ cmake --install build
-- Install configuration: "Release"
CMake Error at build/cmake_install.cmake:46 (message):
  librustls installation via CMake is not supported.  Use 'cargo capi
  install' instead.

  See:
  https://github.com/rustls/rustls-ffi?tab=readme-ov-file#build-rustls-ffi

Resolves #526

@cpu cpu changed the title Cpu cmake tweaks Small CMake related tweaks Jan 20, 2025
@cpu cpu self-assigned this Jan 20, 2025
@cpu cpu marked this pull request as draft January 20, 2025 23:02
@cpu
Copy link
Member Author

cpu commented Jan 20, 2025

cpu marked this pull request as draft now

I had removed the main CI yaml while iterating on this but it looks like I broke the normal CI workflow's build on Windows with the target_link_libraries change. I'll fix this shortly. Folks can hold review until then if desired.

cpu added 3 commits January 22, 2025 16:59
At least one person has convinced themselves CMake is the
required/preferred way to build librustls despite the README documenting
the opposite. Let's try to be even more overt about it.
Previously the artifacts.yaml CI job relied on an external repo that had
a simple C/CMake application depending on librustls. When verifying
artifacts we cloned this repo and tested a build against the artifact.

This commit reworks the main repo's test application code/build to
support this use-case. The main tweak required is to support _not_
building librustls from src, but instead finding it with pkg-config. We
gate this behind a new `FORCE_SYSTEM_RUSTLS` option. The artifact test
CI is updated to use this option.

Annoyingly we have to patch the .pc prefix to an absolute dir to get
this working nicely, which means handling Windows specially.

Similarly, for yet to be determined reasons, on Win32 we have to add
some extra target_link_libraries that don't propagate automatically
through the pc config. For this we just shift the existing Win32
specific target_link_libraries invocation up and use it for both
pkg-config librustls and build-it-ourselves librustls.
This updates the librustls CMakeLists.txt to abort on install. It's only
intended to be used for the C examples, which we _don't_ want anyone to
install anywhere!

Use cargo capi for librustls, not cmake.

```
$ cmake --install build
-- Install configuration: "Release"
CMake Error at build/cmake_install.cmake:46 (message):
  librustls installation via CMake is not supported.  Use 'cargo capi
  install' instead.

  See:
  https://github.com/rustls/rustls-ffi?tab=readme-ov-file#build-rustls-ffi
```
@cpu cpu force-pushed the cpu-cmake-tweaks branch from 5a3852b to 1bdd83e Compare January 22, 2025 22:24
@cpu cpu marked this pull request as ready for review January 22, 2025 22:25
@cpu
Copy link
Member Author

cpu commented Jan 22, 2025

looks like I broke the normal CI workflow's build on Windows with the target_link_libraries change. I'll fix this shortly.

All fixed. Just needed a small adjustment for consistent target_link_options form between two call-sites on the Win32 path.

@ctz If you could spare time for a review it would be appreciated (no big rush). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant