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

doc: document generating bindings with prebuilt libs2n #4872

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

Conversation

jouho
Copy link
Contributor

@jouho jouho commented Nov 6, 2024

Resolved issues:

This is one of the ORR action items to improve documentations on generating rust bindings:

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:

  1. made an edit to s2n_init() function to printout a message indicating pre-built libs2n is being used
  2. compile this s2n-tls
  3. created an empty rust project and add s2n-tls-sys dependency by running cargo add s2n-tls-sys.
  4. include s2n_init() in the main.rs
  5. ran export commands and set the path to libs2n that was built in step 2
  6. ran cargo run which printed out the message added in step 1

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

@github-actions github-actions bot added the s2n-core team label Nov 6, 2024
@jouho jouho marked this pull request as ready for review November 6, 2024 02:00
@@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

  1. you do this be setting environment variables
  2. the environment variables point to ...
  3. the final artifact will then ...

Copy link
Contributor Author

@jouho jouho Nov 6, 2024

Choose a reason for hiding this comment

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

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
@jouho jouho requested a review from jmayclin November 6, 2024 23:43
2. cd into your rust project and set environment variables to libs2n library sources.

This tells the bindings to link to pre-built libs2n when running the build script for s2n-tls-sys
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's include the raw definitions, because I think your example includes some assumptions which might not actually match customer environments.

E.g. call out that S2N_TLS_LIB_DIR points to the folder containing the libs2n.a artifact that you would like s2n-tls-sys to link against.

Also, maybe let's call out whether something is different with static vs dynamic libs?

Copy link
Contributor Author

@jouho jouho Nov 7, 2024

Choose a reason for hiding this comment

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

Added explanation on what each vars are for.

maybe let's call out whether something is different with static vs dynamic libs?

I understand static lib bakes the code to executable at build time and dynamic lib requires the linked libraries to be available at runtime. Is there something else we want to call out within the context of this bindings generation?

bindings/rust/s2n-tls-sys/README.md Show resolved Hide resolved
bindings/rust/s2n-tls-sys/README.md Show resolved Hide resolved
- add new header
- raw definitions for export vars
- mention default built steps
@jouho jouho requested a review from jmayclin November 7, 2024 23:55
```

`S2N_TLS_LIB_DIR` points to the folder containing `libs2n.a`/`lins2n.so` artifact that you would like s2n-tls-sys to link against.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`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.

bindings/rust/s2n-tls-sys/README.md Show resolved Hide resolved
- typo fix
- move build process info up a section
@jouho jouho requested a review from jmayclin November 11, 2024 20:44
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.

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.


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`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I prefer a more "imperative" tone. (use, link) instead of "uses", "links".

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

Successfully merging this pull request may close these issues.

2 participants