Fix rustls-tls feature #1194
Fix rustls-tls feature #1194
Conversation
Commit 14e7487 (cookie support lycheeverse#1146) re-introduced an unconditional dependency on the openssl-sys crate. That is, building Lychee with the Rustls TLS backend now requires OpenSSL. I suppose this change was unintended, maybe due to automatic conflict resolution. If not, please let me know. You can review the re-introduced dependency like so: ``` cargo tree --no-default-features --features rustls-tls -i openssl-sys ``` This commit puts the OpenSSL dependency behind the native-tls feature flag again. You can check the TLS features like so: ``` cargo check --workspace --all-targets --features vendored-openssl cargo check --workspace --all-targets --all-features cargo check --workspace --all-targets --no-default-features --features rustls-tls ``` Maybe this should be added to CI. But I don't want to waste anybody's time.
|
Ah right, nice catch! It certainly wasn't intended, and we should add a check to CI/CD. Want to add it right away? I don't think it will significantly impact the CI runtime, as the crate should be built when we run the check. If it takes too long, we can also move it to the release pipeline. |
stefankreutz
force-pushed
the
fix-rustls-tls-feature
branch
from
8de5c11
to
331373a
Compare
Adds a new CI job 'check-feature-flags' to verify the following: - Lychee with rustls-tls feature only doesn't depend on OpenSSL - Cargo check passes with default features - Cargo check passes with all features - Cargo check passes with rustls-tls feature only
stefankreutz
force-pushed
the
fix-rustls-tls-feature
branch
from
331373a
to
85efbe9
Compare
|
I've added a new CI job to check the TLS feature flags. It looks like it does work on my fork, but I'm not that familiar with Github Actions 😅 |
|
Thanks for adding the test. The build time change is quite significant (80% increase), but it could also be flaky runners. 
Let me run the test once more to get a better picture. I guess we could also move the test into the release pipeline to block the release if it breaks. This way, we wouldn't pay for it during CI with the downside of not discovering the issue during feature development. We discussed that option already, but I'm undecided. Feedback welcome! To be fair, the test runtime has deteriorated anyway in the last few months, as can be seen by checking the timings: |
|
I was mistaken. Seems like Thanks for the fix and for caring about that part @stefankreutz. ⭐ |
Commit 14e7487 (cookie support #1146) re-introduced an unconditional
dependency on the openssl-sys crate. That is, building Lychee with the
Rustls TLS backend now requires OpenSSL. I suppose this change was
unintended, maybe due to automatic conflict resolution. If not, please
let me know.
You can review the re-introduced dependency like so:
This commit puts the OpenSSL dependency behind the native-tls feature
flag again.
You can check the TLS features like so:
Maybe this should be added to CI. But I don't want to waste anybody's
time.