Attempt at providing an owned version of command #136

masklinn · 2023-09-16T17:48:03Z

Pushing this to run the tests as getting things to work locally on a mac seems like a bit of a bear.

Could not get the lifetimes right to not break RemoteChild::session (the lifetime becomes bound to the child's rather than the original session's), so move it into its own impl block, and added an extremely debatable Child::into_session, since it means not being able to call wait, or wait_with_output, or disconnect, so probably not useful (but a slightly separate variant of Child::session seems awful).
Initially wanted to have a version directly owning a Session, but that turns out impossible as the Command builder works off of mutable references, so not possible to consume the builder in order to get the session out, short of creating either separate impl blocks for Command<&Session>, Command<Arc<Session>> and Command<Session>, or separate consuming spawns, neither sounding great.
Naming is dubious, especially shared_command, but I don't recall ever seeing a method around shared refs so I went simple. I would have impl'd Arc<Session>::into_command but that's not legal...

Maybe a free function would be better?

Hopefully will eventually fix #117

jonhoo · 2023-09-16T17:48:09Z

This change is

masklinn · 2023-09-16T18:40:54Z

@NobodyXu can you approve workflow runs? Also maybe comment on this early starting point.

codecov-commenter · 2023-09-16T23:16:53Z

Codecov Report

Merging #136 (bbe309a) into master (499c672) will increase coverage by 0.00%.
The diff coverage is 83.33%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
src/child.rs	`89.74% <100.00%> (ø)`
src/command.rs	`89.92% <76.00%> (ø)`
src/session.rs	`77.19% <86.04%> (+1.88%)`	⬆️

NobodyXu · 2023-09-16T23:17:42Z

P.S. I don't think implementing Command<Session> is a good idea, since it's never designed to be taken by value.

Command<Arc<Session>> or Command<Rc<Session>> is more similar to Command<&Session> and I'd like to see support for them.

src/child.rs

src/session.rs

tests/openssh.rs

masklinn · 2023-09-17T16:15:18Z

P.S. I don't think implementing Command<Session> is a good idea, since it's never designed to be taken by value.

I was mostly thinking of the case where a user might want to run a single command on the session, but didn't want to bother with borrowing. However as I wrote up it doesn't work without changing the Command API entirely so that's moot.

src/command.rs

src/session.rs

src/command.rs

src/lib.rs

masklinn · 2023-09-30T15:41:51Z

moved the trait bound from AsRef to Deref
- made [raw_]command go through to_[raw_]command since they now can
renamed the owning Command to OwningCommand instead of just aliasing it as the crate root, for documentary clarity
added a to_subcommand associated function to do the session-owning thing for subcommand features

src/session.rs

NobodyXu · 2023-10-01T10:57:05Z

BTW, you need to fix the CI before I can merge this PR.

masklinn · 2023-10-01T18:30:59Z

BTW, you need to fix the CI before I can merge this PR.

Yep but since I can't run the tests locally I can only push and wait for CI approval. I'll get on that.

NobodyXu · 2023-10-01T23:36:21Z

@masklinn There's a run_ci_tests.sh in the repository that can be used to run some of the tests.

For linting, it's just cargo-clippy and cargo-fmt.

masklinn · 2023-10-02T14:55:03Z

There's a run_ci_tests.sh in the repository that can be used to run some of the tests.

Yeah but it requires docker, and I'm on a mac and not super willing to install docker desktop. It's nbd as far as I'm concerned just a bit more latency but I'm not replying within seconds anyway. Unless you have to approve every CI run I can imagine that gets old.

For linting, it's just cargo-clippy and cargo-fmt.

Yeah I ran fmt but it changed imports I had not touched so I figured the repo had strange configuration and reverted them, and they show up as lint errors so I guess it's just that they were last fmt'd before a change in configuration or something.

masklinn · 2023-10-02T15:02:59Z

Hopefully CI is good now, I broke one of the existing doc comments by mistake, and the new arc_command example had a few brainfarts.

NobodyXu · 2023-10-02T23:54:48Z

Yeah but it requires docker, and I'm on a mac and not super willing to install docker desktop.

You can use OrbStack, an alternative to docker desktop that claims to be faster than docker desktop and uses less energy.

I use it for docker desktop now and it indeed startup quickly.

It's nbd as far as I'm concerned just a bit more latency but I'm not replying within seconds anyway. Unless you have to approve every CI run I can imagine that gets old.

Yes, I do need to approve every CI run.

Once this PR lands in main, any new PR from you will have CI run automatically without having me to approve.

so I guess it's just that they were last fmt'd before a change in configuration or something.

It's probably because new lints are added to newer cargo.

NobodyXu · 2023-10-03T22:32:09Z

tests/openssh.rs

+
+#[tokio::test]
+#[cfg_attr(not(ci), ignore)]
+async fn generic_client() {


Suggested change

async fn generic_client() {

async fn rc_client() {

I think rc_client is a better name here.

Dunno, the intent of the name was to indicate that we're testing the generic Session::to_command rather than the ref-specific Session::commandor the Arc-specificSession::arc_command`, the Rc is an implementation detail of the test but I figured I might as well use that since references and arc are already exercised. Would it help if I made most of the body generic and then ran it with both Rc and Arc?

Technically I think I could even run it with a reference since the test doesn't actually move the session (or command, or child), just tests that the "original" session can get dropped.

It makes sense then, though I would like to rename this and the other test fn to test_session_to_command and test_session_arc_command

Works for me.

NobodyXu · 2023-10-03T22:35:09Z

The coverage failure is probably due to CI setup (e.g. GHA does not allow PRs from non-member to use secrets) or whatever, I can bypass that and merge the PR once you address my review.

Supporting session-owning commands (and children) makes it easier to e.g. move long-running children around, or store them around in structures, without having to deal with self-reference issues. Currently, support is limited to clonable smart pointers which can be coerced to `&Session`. After some testing / discussion, `Deref` seems like a better bound than `AsRef` as it works with references, and thus (amongst other things) allows the existing `Session::command` and `Session::raw_command` to go through the new associated function. Provide an `arc_[raw_]command` helper as wrapping an `Arc<Session>` to unbound the command (and child) from the session's scope is the primary and likely major use case of this feature. Does not work with a bare `Session` because `Command::spawn`, `Command::output`, and `Command::status` take self by (mutable) reference, so there is no way to consume the `Session` out. Generally `Command` as a builder is set up around chaining mutable references rather than moving ownership. Had to shuffle names a bit to try and avoid breaking backwards compatibility, which is not great: the internal `Command` becomes the external `OwningCommand`, and the old `RemoteChild` is now `Child`, with the external `RemoteChild` being an alias. Also add an owning version of `Session::subsystem` as well, following the same pattern, since while `subsystem` creates a `OwningCommand` it goes through a different path and thus can't be handled by `Session::to_command`.

NobodyXu · 2023-10-08T04:59:26Z

Thank you @masklinn !

NobodyXu · 2023-10-08T11:56:59Z

Released in v0.10.1

NobodyXu requested changes Sep 16, 2023

View reviewed changes

src/child.rs Outdated Show resolved Hide resolved

src/child.rs Outdated Show resolved Hide resolved

src/session.rs Outdated Show resolved Hide resolved

src/session.rs Outdated Show resolved Hide resolved

tests/openssh.rs Outdated Show resolved Hide resolved

NobodyXu requested changes Sep 25, 2023

View reviewed changes

src/command.rs Outdated Show resolved Hide resolved

masklinn force-pushed the owned-remote-child branch from 8eee290 to 5eb30fd Compare September 25, 2023 16:08

masklinn marked this pull request as ready for review September 25, 2023 16:11

NobodyXu requested changes Sep 25, 2023

View reviewed changes

src/session.rs Outdated Show resolved Hide resolved

src/session.rs Outdated Show resolved Hide resolved

src/command.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

masklinn force-pushed the owned-remote-child branch from 5eb30fd to da0da0c Compare September 30, 2023 15:39

NobodyXu requested changes Sep 30, 2023

View reviewed changes

src/session.rs Show resolved Hide resolved

masklinn force-pushed the owned-remote-child branch 2 times, most recently from ff9a548 to 87f8dec Compare October 2, 2023 15:01

masklinn force-pushed the owned-remote-child branch from 87f8dec to 5b3f427 Compare October 3, 2023 18:12

NobodyXu requested changes Oct 3, 2023

View reviewed changes

masklinn force-pushed the owned-remote-child branch from 5b3f427 to 9a26c5f Compare October 7, 2023 15:02

masklinn force-pushed the owned-remote-child branch from 9a26c5f to bbe309a Compare October 7, 2023 15:03

NobodyXu approved these changes Oct 8, 2023

View reviewed changes

NobodyXu merged commit f64b62a into openssh-rust:master Oct 8, 2023
12 checks passed

masklinn deleted the owned-remote-child branch October 8, 2023 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt at providing an owned version of command #136

Attempt at providing an owned version of command #136

masklinn commented Sep 16, 2023 •

edited

Loading

jonhoo commented Sep 16, 2023

masklinn commented Sep 16, 2023

codecov-commenter commented Sep 16, 2023 •

edited

Loading

NobodyXu commented Sep 16, 2023

masklinn commented Sep 17, 2023

masklinn commented Sep 30, 2023

NobodyXu commented Oct 1, 2023

masklinn commented Oct 1, 2023

NobodyXu commented Oct 1, 2023

masklinn commented Oct 2, 2023 •

edited

Loading

masklinn commented Oct 2, 2023

NobodyXu commented Oct 2, 2023

NobodyXu Oct 3, 2023

masklinn Oct 7, 2023 •

edited

Loading

NobodyXu Oct 7, 2023

masklinn Oct 7, 2023

NobodyXu commented Oct 3, 2023

NobodyXu commented Oct 8, 2023

NobodyXu commented Oct 8, 2023

Attempt at providing an owned version of command #136

Attempt at providing an owned version of command #136

Conversation

masklinn commented Sep 16, 2023 • edited Loading

jonhoo commented Sep 16, 2023

masklinn commented Sep 16, 2023

codecov-commenter commented Sep 16, 2023 • edited Loading

Codecov Report

NobodyXu commented Sep 16, 2023

masklinn commented Sep 17, 2023

masklinn commented Sep 30, 2023

NobodyXu commented Oct 1, 2023

masklinn commented Oct 1, 2023

NobodyXu commented Oct 1, 2023

masklinn commented Oct 2, 2023 • edited Loading

masklinn commented Oct 2, 2023

NobodyXu commented Oct 2, 2023

NobodyXu Oct 3, 2023

Choose a reason for hiding this comment

masklinn Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

NobodyXu Oct 7, 2023

Choose a reason for hiding this comment

masklinn Oct 7, 2023

Choose a reason for hiding this comment

NobodyXu commented Oct 3, 2023

NobodyXu commented Oct 8, 2023

NobodyXu commented Oct 8, 2023

masklinn commented Sep 16, 2023 •

edited

Loading

codecov-commenter commented Sep 16, 2023 •

edited

Loading

masklinn commented Oct 2, 2023 •

edited

Loading

masklinn Oct 7, 2023 •

edited

Loading