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

mv api/unstable/*.h -> api/s2n/unstable/*.h #4825

Closed
wants to merge 5 commits into from

Conversation

graebm
Copy link
Contributor

@graebm graebm commented Oct 7, 2024

Issue:

External projects can always do:
#include <s2n.h>

But, for the unstable headers the path is either
#include <s2n/unstable/cleanup.h>
or
#include <unstable/cleanup.h>

...depending on whether s2n was installed before your project, or built at the same time as your project via CMake's add_subdirectory(s2n). CRT language bindings like aws-crt-python build s2n via add_subdirectory.

Description of changes:

  • Move api/unstable/*.h -> api/s2n/unstable/*.h
    • This way, #include <s2n/unstable/XYZ.h> works regardless of how s2n was built
  • Update all code to use new path

Call-outs:

I tried to use a symlink from api/unstable -> api/s2n/unstable, to keep the old paths working too, but it was confusing tools like cp -r and zip. See failed build here.

An alternate fix was attempted here: #4826
It relies entirely on one symlink and changes very few files, but it's failing for the same reason as the symlink mentioned above.

Testing:

  • Ensured unstable headers still install to same location
  • Ensured code still built when using old unstable/*.h paths
  • Ensured code built when using new s2n/unstable/*.h paths

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

@jmayclin jmayclin self-requested a review October 7, 2024 22:01
Copy link
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

Approving since this shouldn't be a breaking change for anyone consuming s2n-tls from as an installed lib, but long term I think we need stronger language discouraging external consumption of s2n-tls from an add_subdirectory context.

add_subdirectory is not compatible with s2n-tls's reliance on configure-time feature probes for libcrypto capability detection.

@jmayclin jmayclin requested a review from lrstewart October 8, 2024 01:35
@graebm
Copy link
Contributor Author

graebm commented Oct 8, 2024

Don't merge this yet. I'll see if I can stop the CRT libs from consuming S2N via add_subdirectory...

@lrstewart lrstewart added the do_not_merge PR might needs something before merging, even if approved and passing label Oct 8, 2024
@graebm
Copy link
Contributor Author

graebm commented Oct 8, 2024

Yeah, closing this.
Didn't realize till I'd written the PR description, that what the CRT libs were doing with add_subdirectory() is downright shenanigans

@graebm graebm closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do_not_merge PR might needs something before merging, even if approved and passing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants