fix(api/unstable): make all api methods visible #4015
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the simplify part is using the headers copied from s2n-tls to the lib directory.
I really just need to add this as a commit hook
camshaft
reviewed
May 23, 2023
bindings/rust/s2n-tls-sys/Cargo.toml
Outdated
Comment on lines
26
to
29
| crl = [] | ||
| fingerprint = [] | ||
| internal = [] | ||
| npn = [] |
There was a problem hiding this comment.
For the unstable features I think we want to prefix them with unstable-, similar to s2n-quic: https://github.com/aws/s2n-quic/blob/21a82e5944465565cc11a53637a127110b6a81e8/quic/s2n-quic/Cargo.toml#L50.
Ideally, we'd have a similar check: https://github.com/aws/s2n-quic/blob/21a82e5944465565cc11a53637a127110b6a81e8/quic/s2n-quic/src/lib.rs#L75-L103
There was a problem hiding this comment.
I added the unstable prefix, but are we sure that we want to add the unstable flag requirement? I've personally always found it very irritating to work with, and think that the unstable prefix should do a sufficient job of warning customers?
toidiu
reviewed
May 23, 2023
toidiu
approved these changes
May 23, 2023
camshaft
reviewed
May 24, 2023
* regex dependency unused * constants have static lifetime by default
camshaft
approved these changes
May 25, 2023
dougch
pushed a commit
to dougch/s2n-tls
that referenced
this pull request
May 31, 2023
* add appropriate includes so that all api methods are visible when s2n-tls is compiled as a shared object * autogenerate bindings for all api methods and test the visibility to prevent a regression
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved issues:
resolves #4012
Description of changes:
cargo test --all-featuresto avoid the multiple builds incurred by different feature flagsThe "external build" test for the Rust bindings CI is a very effective test for api visibility, so I
This gives us coverage over all of the unstable functions, and as such we can conclude that all of the functions in unstable headers will be visible after this commit.
Since the bindings are now autogenerated for ever header in the unstable folder, this will hopefully prevent the problem from occuring in the future.
The methods that weren't actually visible were
Testing:
Passing CI should be sufficient, but I also rebased my JA-3 PR #4009 on top of this PR and made sure that everything built and all tests passed.
I also confirmed that the same number of tests ran (because of the changed type allowlisting thing) and there is actually one test that apparently we were accidentally denylisting, because now 221 tests run instead of 220.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.