doc: document generating bindings with prebuilt libs2n #4872
Conversation
bindings/rust/README.md
Outdated
| @@ -20,6 +20,20 @@ Generating rust bindings can be accomplished by running the `generate.sh` script | |||
| $ ./bindings/rust/generate.sh | |||
| ``` | |||
|
|
|||
| Alternatively, rust bindings can be generated using pre-built libs2n by first compiling libs2n, and then running the `generate.sh` script: | |||
There was a problem hiding this comment.
I think we should be writing this from the perspective of external consumers, who wouldn't be able to run generate.sh. They'd just be pulling the generated s2n-tls-sys from crates.io.
So it's probably a little bit more useful to include a workflow oriented around "how do I call cargo build and use my own libs2n?", and skip the .generate.sh steps.
I think it would also be nice to be a bit more verbose about the process.
E.g.
- you do this be setting environment variables
- the environment variables point to ...
- the final artifact will then ...
There was a problem hiding this comment.
I rewrote the doc from the perspective of crates consumer with more details.
I believe this documentation is also valuable for direct crates consumers, so I added the same information in the readme for s2n-tls-sys crates (assuming this is the readme that is shown on creates page when published). This way, crates consumers are aware of this option when looking from crates.io, and they are also able to find this option at bindings/rust/README even if they are not aware of s2n-tls-sys crate
- document from the perspective of crates consumer - add the same information on crates' readme
- add new header - raw definitions for export vars - mention default built steps
bindings/rust/s2n-tls-sys/README.md
Outdated
| ``` | ||
|
|
||
| `S2N_TLS_LIB_DIR` points to the folder containing `libs2n.a`/`lins2n.so` artifact that you would like s2n-tls-sys to link against. |
There was a problem hiding this comment.
| `S2N_TLS_LIB_DIR` points to the folder containing `libs2n.a`/`lins2n.so` artifact that you would like s2n-tls-sys to link against. | |
| `S2N_TLS_LIB_DIR` points to the folder containing `libs2n.a`/`libs2n.so` artifact that you would like s2n-tls-sys to link against. |
- typo fix - move build process info up a section
There was a problem hiding this comment.
Maybe a copy paste error? It looks like the same text is included twice. I think it should only be in s2n-tls-sys/Readme.md. Although if you think it should be mentioned in the main bindings readme, then you could link bindings/rust/Readme.md to the s2n-tls-sys readme.
bindings/rust/README.md
Outdated
|
|
||
| The `s2n-tls-sys` crate contains the raw C code of `s2n-tls`. By default, it follows this build process: | ||
|
|
||
| 1. Uses the system C compiler to build `libs2n.a` |
There was a problem hiding this comment.
Nit: I prefer a more "imperative" tone. (use, link) instead of "uses", "links".
Yes, I included the same information in the bindings readme. This way, crates consumers are aware of this option when looking from crates.io, and other customers are able to find this option at bindings/rust/README even if they are not aware of s2n-tls-sys crate. Though I think you're right, I will link bindings/rust/Readme.md to the s2n-tls-sys readme. |
- use more imprerative tone - link sys crate binding to the main bindings readme
bindings/rust/README.md
Outdated
| @@ -20,6 +20,9 @@ Generating rust bindings can be accomplished by running the `generate.sh` script | |||
| $ ./bindings/rust/generate.sh | |||
| ``` | |||
|
|
|||
| This script generates the low-level bindings crate `s2n-tls-sys`, which other bindings crates depend on. | |||
There was a problem hiding this comment.
Nit: Your phrasing is weird here.
| This script generates the low-level bindings crate `s2n-tls-sys`, which other bindings crates depend on. | |
| This script generates low-level bindings in the crate `s2n-tls-sys`, which is used by the `s2n-tls` crate to provide higher-level bindings. |
bindings/rust/s2n-tls-sys/README.md
Outdated
| @@ -1,3 +1,43 @@ | |||
| This crates provides low level rust bindings for [s2n-tls](https://github.com/aws/s2n-tls) which are autogenerated with [bindgen](https://github.com/rust-lang/rust-bindgen) | |||
There was a problem hiding this comment.
| This crates provides low level rust bindings for [s2n-tls](https://github.com/aws/s2n-tls) which are autogenerated with [bindgen](https://github.com/rust-lang/rust-bindgen) | |
| This crate provides low level rust bindings for [s2n-tls](https://github.com/aws/s2n-tls) which are autogenerated with [bindgen](https://github.com/rust-lang/rust-bindgen) |
Since you're here...
bindings/rust/s2n-tls-sys/README.md
Outdated
| This crate is not intended for direct consumption by end consumers. Interested developers should instead look at the [s2n-tls](https://crates.io/crates/s2n-tls) or [s2n-tls-tokio](https://crates.io/crates/s2n-tls-tokio) crates. These provide higher-level, more ergonomic bindings than the `s2n-tls-sys` crate. |
There was a problem hiding this comment.
| This crate is not intended for direct consumption by end consumers. Interested developers should instead look at the [s2n-tls](https://crates.io/crates/s2n-tls) or [s2n-tls-tokio](https://crates.io/crates/s2n-tls-tokio) crates. These provide higher-level, more ergonomic bindings than the `s2n-tls-sys` crate. | |
| This crate is not intended for direct consumption by end consumers. Interested developers should instead look at the [s2n-tls](https://crates.io/crates/s2n-tls) or [s2n-tls-tokio](https://crates.io/crates/s2n-tls-tokio) crates. These provide higher-level, more ergonomic bindings than the `s2n-tls-sys` crate. |
bindings/rust/s2n-tls-sys/README.md
Outdated
| The `s2n-tls-sys` bindings crate contains the raw C code of `s2n-tls`. By default, it follows this build process: | ||
|
|
||
| 1. Links the system C compiler to build `libs2n.a` |
There was a problem hiding this comment.
You misunderstood James' comment. This should be "Uses the system C compiler to build `libs2n.a"
There was a problem hiding this comment.
Do you mean it should be "Use the system C compiler to build libs2n.a"? "Uses the system C compiler to build libs2n.a`" is the original phrase I had there
There was a problem hiding this comment.
I don't really care if it's "uses" or "use", just make it match the rest of your list. The verb "links" is wrong.
bindings/rust/s2n-tls-sys/README.md
Outdated
| 3. Links against `aws-lc` through the `aws-lc-rs` crate | ||
|
|
||
| ## Bring Your Own libs2n with `s2n-tls-sys` crate |
There was a problem hiding this comment.
| ## Bring Your Own libs2n with `s2n-tls-sys` crate | |
| ## Bring your own libs2n with `s2n-tls-sys` crate |
bindings/rust/s2n-tls-sys/README.md
Outdated
| ## Bring Your Own libs2n with `s2n-tls-sys` crate | ||
|
|
||
| You can customize above build process to use your own pre-built libs2n library. Here's how you can do that: |
There was a problem hiding this comment.
| You can customize above build process to use your own pre-built libs2n library. Here's how you can do that: | |
| You can customize above build process to use your own pre-built libs2n. Here's how you can do that: |
Nit: "libs2n" means the s2n-tls library, so you don't really need to specify "libs2n library". It's redundant.
bindings/rust/s2n-tls-sys/README.md
Outdated
| Also see [Building s2n-tls](https://github.com/aws/s2n-tls/blob/main/docs/BUILD.md#building-s2n-tls) for further guidance on configuring s2n-tls for your own use case. | ||
| ``` | ||
| cmake . -Bbuild -DBUILD_SHARED_LIBS=on -DBUILD_TESTING=off |
There was a problem hiding this comment.
Hmmm. I don't think you need to include instructions for building s2n if you've already linked to our build page? Is there a specific cmake flag that a user would need to turn on to create the prebuilt libs2n?
There was a problem hiding this comment.
No, static/shared shouldn't matter in linking the prebuilt libs2n. I will remove this instruction.
bindings/rust/s2n-tls-sys/README.md
Outdated
| ``` | ||
|
|
||
| 2. `cd` into your rust project and set environment variables to your libs2n library sources. |
There was a problem hiding this comment.
| 2. `cd` into your rust project and set environment variables to your libs2n library sources. | |
| 2. `cd` into your rust project and set environment variables to your libs2n artifacts. |
bindings/rust/s2n-tls-sys/README.md
Outdated
| ``` | ||
|
|
||
| This method is useful if you want the bindings to be built with a non-default libcrypto. Currently, the default libcrypto when generating rust bindings is `aws-lc`. |
There was a problem hiding this comment.
It's weird to put this note at the bottom of the page. You want motivation for "why" to run these commands to come before the instructions, not after.
- fix wrong verb - remove unnecessary intstruction - fix nits
Resolved issues:
The Rust bindings support usage with a customer supplied libs2n, but that option is very poorly documented.
We should add public documentation for it.
Description of changes:
Documented an alternative way to generate bindings using pre-built libs2n, and provided an example code snippet for it. Added this to both bindings/rust/README and bindings/rust/s2n-tls-sys/README
Call-outs:
Testing:
Ran the documented commands locally and confirmed it triggers build process with prebuilt libs2n. The way I confirmed this is:
cargo add s2n-tls-sys.cargo runwhich printed out the message added in step 1By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.