-
Notifications
You must be signed in to change notification settings - Fork 151
Drop libthemis-src support #691
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
Drop libthemis-src support #691
Conversation
RustThemis has a `libthemis-src` crate which can be enabled with `vendored` feature. It will build Themis library on-the-fly, during `themis` build. It was a nice idea in the beginning, aimed at making developers' lives easier. However, it also has some issues and maintenance overhead: * This is yet another configuration. There are tests for it, but they don't cover actual `themis` execution in this configuration, only `libthemis-src` build itself. * Those tests may cause false positives because they are run from the repository root. In fact, currently published `libthemis-src = 0.13.0` does not build because some files are missing from it. * We hijack a lot of decisions from the developers: how to link against Themis and what cryptography backend to use. `libthemis-src` does not offer a choice of the backend, it's always system OpenSSL. * Building `libthemis-src` requires a tricky repository layout with symlinks that point to Themis source code. Every time a new build dependency is added, maintainers need to make sure that all dependencies are symlinked. * `cargo publish` does not handle symlinks well and it may break publishing at times, when symlinks are included into the crate, not the files they point to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR looks good, but I believe there is one use case – building API docs on docs.rs – which will be broken if the changes are merged as is. However, there seems to be fix that needs to be tested. @iamnotacake, could you please look into it?
src/wrappers/themis/rust/Cargo.toml
Outdated
[package.metadata.docs.rs] | ||
features = ["vendored"] | ||
dependencies = ["libssl-dev"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the original reasons why libthemis-src
was added (see #349 and ilammy/rust-themis#9). The thing is, docs.rs does not have Themis installed so if the build requires Themis to be installed, then docs.rs will not be able to build API documentation.
I suspect that it's no longer necessary given that libthemis-sys
now does not use Bindgen to generate bindings on-the-fly. However, I have checked and it seems that we still (kinda) need Themis to be installed because build.rs
of libthemis-sys
checks that Themis is installed, even when just cargo doc
is run.
We can't remove libthemis-src
without fixing up docs.rs generation. I have looked around and it seems that they set DOCS_RS
environment variable when generating documentation. We can use it in libthemis-sys/build.rs
: look at that variable, if it is 1
then do not detect Themis installation and exit right away.
That way docs.rs will be able to run cargo doc
without Themis being installed. In fact, we can remove this entire section because we don't need OpenSSL to be installed if we aren't going to build Themis.
It would also be nice to test this on CI somehow, but I don't see an immediate approach to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked, with
if std::env::var("DOCS_RS") == Ok("1".to_string()) {
return;
}
in libthemis-sys/build.rs -> main()
, DOCS_RS=1 cargo doc
really works w/o libthemis installed, while running only cargo doc
fails as expected. BTW, do we still need
[package.metadata.docs.rs]
dependencies = ["libssl-dev"]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOCS_RS=1 cargo doc
really works w/o libthemis installed
Amazing! I believe that should do the trick.
do we still need
dependencies = ["libssl-dev"]
No, I believe the entire [package.metadata.docs.rs]
block can be removed. libssl-dev
was required for Themis to compile, but if it's not going to be compiled then we don't need OpenSSL either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice to test this on CI somehow, but I don't see an immediate approach to that.
Tried passing different PKG_CONFIG_*
env vars (described in man pkg-config
) to cargo build
to make the compilation fail while the libs are installed. No success. If so, I would add something like
cargo clean --doc && DOCS_RS=1 PKG_CONFIG_SEARCH_ONLY_IN=/nonexistingdir cargo doc --workspace --no-deps
to run_tests.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another issue with CI is that libthemis-sys will try the system installation if pkg-config fails to find Themis Code. CI builds and installs Themis Core into the system before running RustThemis tests. We will need to somehow uninstall it for the docs.rs tests and then install it back for other tests. Maybe a better idea will present itself later. For now, I think, we can leave it like that. It's enough that you have tested it manually.
* Rewrite description of changes in changelog * Don't search for Themis libs if env var DOCS_RS is 1 (it is set to 1 when docs are generated for https://docs.rs website)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! 👍
If you say that it works, I trust you :) |
RustThemis has a
libthemis-src
crate which can be enabled withvendored
feature. It will build Themis library on-the-fly, duringthemis
build. It was a nice idea in the beginning, aimed at makingdevelopers' lives easier. However, it also has some issues and
maintenance overhead:
don't cover actual
themis
execution in this configuration, onlylibthemis-src
build itself.repository root. In fact, currently published
libthemis-src = 0.13.0
does not build because some files are missing from it.
Themis and what cryptography backend to use.
libthemis-src
does notoffer a choice of the backend, it's always system OpenSSL.
libthemis-src
requires a tricky repository layout withsymlinks that point to Themis source code. Every time a new build
dependency is added, maintainers need to make sure that all
dependencies are symlinked.
cargo publish
does not handle symlinks well and it may breakpublishing at times, when symlinks are included into the crate, not
the files they point to.
Checklist