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

Drop libthemis-src support #691

Conversation

iamnotacake
Copy link
Contributor

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.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Changelog is updated (in case of notable or breaking changes)

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.
@iamnotacake iamnotacake added the W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates label Aug 6, 2020
@ilammy ilammy added the compatibility Backward and forward compatibility, platform interoperability issues, breaking changes label Aug 6, 2020
Copy link
Collaborator

@ilammy ilammy left a 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?

Comment on lines 38 to 39
[package.metadata.docs.rs]
features = ["vendored"]
dependencies = ["libssl-dev"]
Copy link
Collaborator

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.

Copy link
Contributor Author

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"]

?

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

CHANGELOG.md Outdated Show resolved Hide resolved
* 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)
Copy link
Collaborator

@ilammy ilammy left a comment

Choose a reason for hiding this comment

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

Good job! 👍

@vixentael
Copy link
Contributor

If you say that it works, I trust you :)

@iamnotacake iamnotacake merged commit 86d9e70 into cossacklabs:master Aug 6, 2020
@iamnotacake iamnotacake deleted the anatolii-T1711-drop-libthemis-src-support branch August 6, 2020 16:24
@ilammy ilammy mentioned this pull request Aug 17, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Backward and forward compatibility, platform interoperability issues, breaking changes W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants