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

Support gating libssh-rs and ssh2 behind features of same name #1415

Merged
merged 2 commits into from
Jan 10, 2022

Conversation

chipsenkbeil
Copy link
Contributor

@chipsenkbeil chipsenkbeil commented Dec 18, 2021

Moves libssh-rs and ssh2 backends to be gated by features of the same name. This enables building wezterm-ssh with only one of two dependencies, getting around problems with compiling both for a cdylib target.

You can verify that each backend works by running with cargo test --no-default-features --features libssh-rs and cargo test --no-default-features --features ssh2 from within the wezterm-ssh directory. If libssh-rs is not present, then the backend will be switched to ssh2.

@chipsenkbeil
Copy link
Contributor Author

chipsenkbeil commented Dec 18, 2021

@wez the only thing not done right now is how to handle the vendored-openssl feature. With stable cargo, specifying the feature of a dependency as enabled will always include that feature, even if the dependency itself hasn't been included.

There was an issue about it rust-lang/cargo#3494, with a solution called weak dependency features merged in last year, but this feature is still only available on nightly cargo.

For now, I've added two features to enable vendored-openssl individually for ssh2 and libssh-rs.

@chipsenkbeil
Copy link
Contributor Author

@wez if there's any early comments you might have, let me know :) I'm assuming everyone is taking a break throughout the holidays, so not expecting a full review until next year. This is one of the blockers for the next release of distant, so would love to get this rolling, though!

@wez wez merged commit 5042add into wez:main Jan 10, 2022
@wez
Copy link
Owner

wez commented Jan 10, 2022

Looks good, thanks!

@chipsenkbeil
Copy link
Contributor Author

@wez just noticed that the weak feature functionality of cargo is now released: https://blog.rust-lang.org/2022/04/07/Rust-1.60.0.html#new-syntax-for-cargo-features.

If/when you bump to the new cargo version, you could collapse the old features above, removing the need for features like vendored-openssl-ssh2, vendored-openssl-libssh-rs. Instead, you'd have vendored-openssl = ["ssh2?/vendored-openssl", "libssh-rs?/vendored-openssl"].

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.

2 participants