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

Allow conversion of std::process::Command into a openssh::Command given a openssh::Session. #112

Merged
merged 27 commits into from
Jul 28, 2023

Conversation

aalekhpatel07
Copy link
Contributor

@aalekhpatel07 aalekhpatel07 commented Jan 31, 2023

The std::process::Command and the openssh::Command APIs are fairly similar, so I wonder if it is reasonable to allow an arbitrary std::process::Command to be run over ssh.

Example

use std::process;
use openssh::{OverSsh, Session, KnownHosts};

#[tokio::main]
pub async fn main() {
    let session = Session::connect_mux("[email protected]", KnownHosts::Strict).await.unwrap();

    let ls = process::Command::new("ls")
        .arg("-l")
        .arg("-h")
        .over_ssh(&session)
        .expect("no env vars or current_dir are specified")
        .output()
        .await
        .unwrap();

    println!("{}", String::from_utf8_lossy(&ls.stdout));
}

@jonhoo
Copy link
Collaborator

jonhoo commented Jan 31, 2023

This change is Reviewable

Copy link
Member

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks!

I really like the design and the implementation is simple, but there are a few places where it can be improved.

src/command.rs Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
@aalekhpatel07
Copy link
Contributor Author

wow thank you for your kind words and a quick review. I'll clean it up right away! Thanks for your time.

src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
@NobodyXu NobodyXu requested a review from jonhoo January 31, 2023 05:21
@NobodyXu
Copy link
Member

Ehhh we now have a rustc ICE.

@aalekhpatel07 Would you like to open an issue in rust-lang/rust?

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2023

Codecov Report

Merging #112 (f30d34e) into master (3319a81) will increase coverage by 0.32%.
The diff coverage is 87.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
Files Changed Coverage Δ
src/error.rs 89.16% <ø> (ø)
src/command.rs 89.92% <69.23%> (-8.08%) ⬇️
src/escape.rs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@aalekhpatel07
Copy link
Contributor Author

Ehhh we now have a rustc ICE.

@aalekhpatel07 Would you like to open an issue in rust-lang/rust?

Haha for sure. Any preference for a name?

@NobodyXu
Copy link
Member

@aalekhpatel07 Looks like some of the ICEs happens in mir borrow checking, so perhaps "mir borrow checking ICE in openssh-rust" will be a good name.

Also I recommend you to create a separate branch, containing the commit causing the ICEs just so it's easier for others to reproduce the ICEs.

@aalekhpatel07 aalekhpatel07 requested review from NobodyXu and removed request for jonhoo January 31, 2023 06:11
@aalekhpatel07
Copy link
Contributor Author

aalekhpatel07 commented Jan 31, 2023

I believe the new escape(s: &[u8]) -> String should do the job for us. lmk

(I'm sorry I think I accidentally removed the review request for jonhoo).

@NobodyXu NobodyXu requested a review from jonhoo January 31, 2023 06:17
src/command.rs Outdated Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Member

(I'm sorry I think I accidentally removed the review request for jonhoo).

No worries, I've re-requested it.

@NobodyXu
Copy link
Member

Aha, the CI reliably failed with ICEs from rustc.

@aalekhpatel07
Copy link
Contributor Author

okay how about these changes? (thank you for being patient.)

src/command.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/escape.rs Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
src/escape.rs Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Member

okay how about these changes? (thank you for being patient.)

It's much better, but we would still need @jonhoo 's approval on this.
I'm not entirely sure about escaping since the utf encoding is a mess, so I just want to be safe and ask @jonhoo to also review this.

@NobodyXu
Copy link
Member

NobodyXu commented Jan 31, 2023

@aalekhpatel07 BTW, don't forget to create a new issue on rust-lang/rust and linked it to this.
We would also have to waiting for the compiler guys to investigate and fix the issues.

Since we are on rust 1.67, not nightly channel, this might requires a new minor release.

I think this ICE might be a regression.

@NobodyXu
Copy link
Member

@aalekhpatel07 Hmmm, let me create the bug report for this.

@NobodyXu
Copy link
Member

@aalekhpatel07 I've created an ICE bug report on rust-lang/rust.

Note that the lint CI did work as intended and you need to fix all cargo-clippy warnings.

Copy link
Member

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Almost done from me, some small changes requested...

Regarding the ICEs, there's already a PR in rust-lang/rust to fix this.

src/escape.rs Outdated Show resolved Hide resolved
src/escape.rs Show resolved Hide resolved
src/escape.rs Outdated Show resolved Hide resolved
Copy link
Member

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Now we just need to wait for @jonhoo to review this and upstream to fix the ICEs on stable channel.

@aalekhpatel07
Copy link
Contributor Author

They have a PR for that ICE already??

Wow! I'm blown away by how responsive, knowledgeable, and supportive the contributors are in the Rust ecosystem.

Thank you for all your help, @NobodyXu, for helping me improve the PR and develop my understanding of Cows and OsStrs in Rust.

@NobodyXu
Copy link
Member

NobodyXu commented Feb 1, 2023

They have a PR for that ICE already??

Yes, rust-lang/rust#107532

Though I have no idea on whether it will land in 1.68 or will they have a backport it to 1.67.1 since whether a backport will happen depends on the severity of the bug.

Thank you for all your help, @NobodyXu, for helping me improve the PR and develop my understanding of Cows and OsStrs in Rust.

No worries!

Copy link
Collaborator

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

This is a really neat idea! Thank you both for pushing it forward. I've left some notes.

src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,90 @@
//! Escape characters that may have special meaning in a shell, including spaces.
//! This is a modified version of the [`shell-escape::unix`] module of [`shell-escape`] crate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we submit a PR to shell-escape to add a version supporting OsStr there directly? I'm okay with us landing our own version initially, but it'd be way better if we could just use a version from them in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! I've initiated a PR to shell-escape and if it gets merged, we can avoid having a src/escape.rs altogether.

src/escape.rs Outdated Show resolved Hide resolved
tests/openssh.rs Outdated Show resolved Hide resolved
aalekhpatel07 and others added 27 commits July 28, 2023 17:54
Signed-off-by: Aalekh Patel <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Co-authored-by: Jiahao XU <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
@NobodyXu
Copy link
Member

Since the ICE is fixed and the CI has passed, I will merge this PR.

@NobodyXu NobodyXu merged commit fefd900 into openssh-rust:master Jul 28, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants