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

Downgrade our vendored OpenSSL version back to 1.1.1 #1578

Merged
merged 1 commit into from
Dec 12, 2021

Conversation

sfackler
Copy link
Owner

3.0.0 unfortunately seems to have shipped with several major performance regressions. It seems best to temporarily revert the version used by the vendored feature back to 1.1.1 for now. Hopefully an upcoming 3.x.x release will fix the performance issues, at which point we'll switch back.

@sfackler sfackler merged commit 89d2ee5 into master Dec 12, 2021
@sfackler sfackler deleted the downgrade-vendored branch December 12, 2021 02:03
wez added a commit to wez/libssh-rs that referenced this pull request Dec 13, 2021
We need the engine changes that are present in openssl-src-300,
which were last in openssl-sys 0.9.71.

We can remove this constraint when the problem that produced this
rollback is resolved.

refs: sfackler/rust-openssl#1578
wez added a commit to wez/wezterm that referenced this pull request Dec 13, 2021
A recent cargo update caused openssl-sys to do a minor semver update
from 0.9.71 -> 0.9.72, but that release downgraded from openssl 3
to openssl 1 to resolve a performance regression:
<sfackler/rust-openssl#1578>

That in turn caused libssh to fail to build because the ENGINE
feature required by libssh isn't compiled in in openssl-src 1
crate when vendoring on windows.

For now, my libssh git repo is constrained to openssl-sys 0.9.71,
and we're pointing to that from the wezterm repo.
@bdbai
Copy link
Contributor

bdbai commented Dec 13, 2021

May I have some details about the performance regression?

Our work requires build support for Windows UWP targets which is introduced in OpenSSL 3, therefore I have to pin version 0.9.71 as a workaround. Would it be better if a feature can be used to allow users to opt-in for OpenSSL 3?

@sfackler
Copy link
Owner Author

sfackler commented Dec 13, 2021

Some examples:
openssl/openssl#16871
openssl/openssl#15199
openssl/openssl#17064

The vendored feature is intended to just provide a reasonable version for people that want to get a thing building - I don't think we'll want to formally support multiple variants of it for different versions. If you have specific needs there, I'd recommend not using the vendored feature and instead point openssl-sys to an existing install that you manage.

@bdbai
Copy link
Contributor

bdbai commented Dec 14, 2021

@sfackler Thanks for the information! Reusing a prebuilt OpenSSL from vcpkg sounds like a good idea for my case.

wez added a commit to wez/wezterm that referenced this pull request May 9, 2022
note: this pins the openssl crate to workaround the combination
of these two issues:

refs: sfackler/rust-openssl#1630
refs: sfackler/rust-openssl#1578
@wez
Copy link

wez commented May 9, 2022

The vendored feature is intended to just provide a reasonable version for people that want to get a thing building

If the vendored feature is aimed to make it convenient to build, rather than the recommended way to pull in the best available openssl, could we revisit and reverse this PR?

In wezterm I use the vendored feature for macOS and Windows. While wezterm itself doesn't have a preference on the openssl version, one of the deps is https://github.com/wez/libssh-rs/tree/main/libssh-rs-sys which needs a build of openssl that includes the engine related functions that are only built on windows starting in openssl-src-300 and up: alexcrichton/openssl-src-rs#109

wez/libssh-rs@3dc392c pinned libssh-rs to openssl-sys =0.9.71 to workaround #1578 which was "fine" until I ran cargo update in wezterm today and ran into #1630. To resolve that, I need to pin wezterm to openssl =0.10.38.

I'd love to remove this pinning! However, it is a huge PITA to do this by providing my own openssl build (as suggested in the comments above), as I'd need to contrive for this to happen for the various combinations/permutations of:

  • CI, non-CI
  • Windows, macOS
  • libssh-rs, wezterm

It seems to me that the vendored feature is ideal for resolving this sort of situation, and that if performance were a motivating factor, that would be a good reason to not use the vendored feature and instead use either the system openssl or some other externally provided build.

What do you think about this? :)

@chipsenkbeil
Copy link

chipsenkbeil commented May 13, 2022

Want to echo that this was an unexpected breakage to distant and indirectly distant.nvim, which consumes libssh-rs and ssh2-rs and uses the vendored feature of rust-openssl underneath.

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.

4 participants