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

Add a vendored feature to both the botan and botan-sys crates #15

Merged
merged 3 commits into from
Sep 27, 2020

Conversation

breard-r
Copy link
Contributor

This vendored feature works the same way as the one in the openssl crate. When activated, it uses the newly added botan-src crate to build Botan as a static library and then links to it instead of a dynamic one.

I guess this feature is, at least partly, what is expected in #14.

Please note that the Botan source code is included as a git submodule linked to its GitHub repository. When a new version of Botan is released, this submodule should be updated to the new tag, the crate version updated and then published.
About the crate version, I used the same format as the openssl-src crate: Botan's version is included in the metadata. This metadata is purely informational, it's here so one can quickly see which Botan version he's getting with the crate.

This work is obviously a start, many things can be improved. The two most important things I can see are the following: prevent the systematic compilation of Botan and allow the user to properly configure the build. For the later, I think the best way to do it is to use environment variables. I first thought os using features, but they are really not adapted to many situations.

This vendored feature works the same way as the one in the openssl
crate. When activated, it uses the newly added botan-src crate to build
botan as a static library and then links to it instead of to a dynamic
one.

Related to randombit#14
@est31
Copy link
Contributor

est31 commented Aug 2, 2020

👍 I want to add botan as one of the verifier libraries of rcgen but I dont want to burden people with installing a package just for running rcgen locally.

Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

I'm generally OK with this but would like to get the cross compilation issue addressed. I'm not sure how hard that would be to do. If there are changes we can make on the Botan build system to make it easier I'm fine with going that route.

configure.arg("--build-targets=static");
configure.arg("--without-documentation");
#[cfg(debug_assertions)]
configure.arg("--debug-mode");
Copy link
Owner

Choose a reason for hiding this comment

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

This not only enables debug assertions but also disables optimization - intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's activated only when the crate is built in debug mode.


fn configure(build_dir: &str) {
let mut configure = Command::new("python");
configure.arg("configure.py");
Copy link
Owner

Choose a reason for hiding this comment

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

This seems to be ignoring cross compilation - the build will be configured for whatever the underlying host system is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added support for configuration options via environment variables. Cross compilation can now be used this way.

@@ -0,0 +1,23 @@
[package]
name = "botan-src"
version = "0.1.0+2.15.0"
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather the crate version make the underlying version visible by packing the major minor and patch versions of Botan into the crate's minor version eg here "0.21500.0" where we assume the Botan minor and patch versions will never be > 99, and then the final patch version is reserved for the crate itself.

Maybe there is some problem with this, I haven't done any vendoring of sources in this manner in Rust. But having the crate match "0.1" regardless of what the underlying library is pinned to seems bad to me also.


[[example]]
name = "build"
path = "examples/build.rs"
Copy link
Owner

Choose a reason for hiding this comment

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

Ideally there would also be a simple test that just verifies that something links, eg invoke botan_ffi_api_version and verify it returns a positive integer.

The underlying Botan version is not included as the crate's minor
version. Botan's minor and patch numbers are represented on two digits.
@randombit
Copy link
Owner

I tested it, it seems to work 🚀

Thank you for your contribution and also your patience.

@randombit randombit merged commit bf0f2fa into randombit:master Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants