Use duct internally?

[killercup]

I just saw this announcement of duct, which seems to handle a bunch of weird edge cases in relation to std::process::Command.

It looks like we can use it to:

• Define commands as strings using its sh fn. Internally it actually calls /bin/sh/cmd.exe, so it doesn't have to do crazy string splitting (it also has a macro). Deprecate macro in favor of clever Assert::command(&str)? #25
• Set working dir Setting command working directory #28
• Set env vars Setting environment variables #27
• pipe stuff into other commands 😮
• Pipe stuff (bytes) into the program Piping (stdin) to the process #26

I really like our fluent API, and would suggest adding methods that internally handle duct::Expression.

cc @nathanross @colin-kiegel @epage who are the authors of the linked issues
cc @oconnor663 who is the author of duct

[oconnor663]

@hniksic also launched https://github.com/hniksic/rust-subprocess recently, which uses the style of the Python standard library. It might be worth looking into both.

[hniksic]

Thanks for the mention, @oconnor663. rust-subprocess provides a lower-level API in the style of Python's subprocess.Popen, with modifications to make it better fit Rust, and a higher-level API in the vein of std::process::Command, with the addition of pipelines created with |.

The initial goal of rust-subprocess was to eliminate the limitations of std::process::Command, such as the lack of 2>&1, and the inability to construct native pipelines. It initially provided the familiar and time-tested subprocess style design, but it quickly became apparent that a builder style API is more Rustic for casual use, so I included that as well. I wasn't even aware at the time that a Rust port of duct existed.

[killercup]

Since creating this issue, I've used duct in a few more projects and really liked it (I still have to use subprocess, but duct seems more popular overall). At the same time, we added a bunch of cool new stuff to assert_cli. Thus, I want to get back to the original question here, but also add this:

Would it make sense to add duct not only as a private dependency to assert_cli, but also extend our API to implement our assertions on duct::Expression?

I imagine this might work (uses current API):

#[macro_use] extern crate duct;
extern crate assert_cli;
use assert_cli::CliAsserts; // trait that extends duct::Expression

#[test]
fn foo() {
    cmd!("echo", "hi").pipe(cmd!("sed", "s/h/p/"))
    .assert().fails().stderr().contains("foo-bar-foo")
}

What do you think? @epage

[oconnor663]

Can I abuse this thread for feedback? I so rarely get any :)

• I don't think anyone uses the then operator, and removing it would make the code a lot simpler. Does anyone object to removing it?

• I think stdout_capture/stderr_capture are a bit awkward to use, since if I'm typing one I'm usually typing both, and I'm thinking about how to simplify them (maybe a unified capture method?).

• @idolf and I have been thinking (Add unix::ExpressionExt containing a uid and gid function oconnor663/duct.rs#56) about adding an interface to construct an Expression from an arbitrary std::process::Command. That would be useful for getting at niche features like before_exec, though it'll probably lead to a quirky precedence swap if you set e.g. stdout in both the underlying command and in the duct expression. (Normally "inner" modifiers in duct take precedence over "outer" modifiers, but in this case duct wouldn't have any way of knowing that you'd set stdout already.) Would you guys find a constructor like that useful, maybe for some of the cases where you reach for subprocess now?

• Is the whole inner-vs-outer-tree-structure precedence thing just too confusing? It has the advantage that if you insert parentheses around your method calls, you can get a good idea of what's happening. But I worry that expressions like this are just always going to surprise people:

  cmd!("foo").env("NAME", "val").env_remove("NAME")
  

  The env_remove there has no effect, because it's "outer".

[epage]

@oconnor663 looks like most of that feedback is targetted at duct and not assert_cli, or am I missing something?

@killercup

Would it make sense to add duct not only as a private dependency to assert_cli, but also extend our API to implement our assertions on duct::Expression?

So assert_cli current provides

• fluent API to spawn a process
• fluent API to assert on said process

duct provides

• fluent API to spawn a process

That does seem like some overlap that we can reduce.

Some counter points

• By the fact that duct tries to do everything, it makes it feel like a heavier weight dependency (no clue if its accurate) and I try to watch for things that will increase my compile times
• Our environment handling is a bit richer but I assume duct could expand to address that.

We could make CliAsserts a trait that is implemented on both duct and Command. If anyone desires the current assert_cli client spawn API (maybe me?), then we could look into splitting that out into a separate crate that also supports the CliAsserts trait.

pub enum OutputAssert {
    output: process::Output
}

impl OutputAssert {
    pub fn new(output: process::Output) -> Self {
        ...
    }

   ... 
}

trait CommandAssert {
    fn assert(self) -> Result<OutputAssert> {
        ...
    }
}

#[cfg(duct)]
impl CommandAssert for duct::Expression {
    ... call run...
}

impl CommandAssert for process::Command {
    ... setup stdout/stderr for reading and call spawn / wait_with_output...
}

Going a step further, we could make our asserts be extension traits to process::Output like cli_test_dir does.

[epage]

Going a step further, we could make our asserts be extension traits to process::Output like cli_test_dir does.

On the surface, duct and this might seem to negate the need to have a CommandAssert trait. The value of it is ensuring Output is setup appropriately for the asserts to successfully run.

Granted, that last part might be a reason not to extend process::Output at all. Currently, we leverage the type system to ensure everything is compatible. We'd lose this advantage.

[epage]

Another question to consider along these lines.

We'll probably need to define this so that when the user starts adding assertions, the process has already run (because we don't have a way to track our extra state otherwise).

Options

• our assertions panic
  • No more need to call unwrap
  • Like I said above, really we are providing a fluent API for checking the result of a process. This could be used outside of testing context where panics are less welcome
• our assertions return Result
  • Unless we duplicate our assertion interface on a Result extension trait, this will make it more annoying to chain and unwrap in a way to show a nice message
• our assertion builder evaluates immediately but internally tracks the error for when the user calls unwrap on it
  • "fast" because we're not building up state and can short circuit later tests
  • simple implementation
• our assertion builder doesn't evaluate anything until unwrap
  • This is closest to what we have today