change stdin to piped

[dakom]

See sagiegurari/cargo-make#355

[codecov-io]

Codecov Report

Merging #9 into master will decrease coverage by 0.38%.
The diff coverage is 76.92%.

@@            Coverage Diff            @@
##           master      #9      +/-   ##
=========================================
- Coverage   90.29%   89.9%   -0.39%     
=========================================
  Files           6       6              
  Lines         309     317       +8     
=========================================
+ Hits          279     285       +6     
- Misses         30      32       +2

Impacted Files	Coverage Δ
src/lib.rs	100% <ø> (ø)	⬆️
src/types.rs	72.72% <100%> (+4.3%)	⬆️
src/types_test.rs	100% <100%> (ø)	⬆️
src/runner.rs	82.39% <62.5%> (-0.94%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09d9a0e...b8b6ec7. Read the comment docs.

[sagiegurari]

I ran a simple test in which i created a task that reads user input as follows:

[tasks.input]
script =[
'''
echo reading..
read vv
echo read $vv
'''
]

with pipe this breaks so its not working well.
we either need a different solution or to enable to set it from outside.
the ability to read user input is critical in case commands require user password (for example if we do sudo).

[dakom]

making it a config option, and having the default be inherit, sounds good to me!

[dakom]

how about making the config options an enum that accepts Null, Pipe, or Inherit? (default will be inherit)

[dakom]

will add to the PR - one min

[sagiegurari]

and what in cargo-make will tell you to choose that or the default?

[dakom]

thinking instead of capture_output being a bool, it could be a string accepting "null", "inherit", or "pipe" - and then add a new one doing the same for capture_input

[dakom]

added to the PR... lemme know what you think :)

[sagiegurari]

don't think you understood my point.

1. pipe breaks the script invocation
2. what would in cargo make tell runscript if to use pipe or not. its ok that runscript allows external flag but what is different between my read input script that breaks and you npx script that hangs that will make cargo-make provide a different value.
3. capture output shouldn't control input. its hacky.

[dakom]

1. pipe breaks the script invocation

depends on what you're running, no? in some scenarios inherit breaks things too, which is why I'm saying to make it configurable but leave the default as inherit

2. what would in cargo make tell runscript if to use pipe or not. its ok that runscript allows external flag but what is different between my read input script that breaks and you npx script that hangs that will make cargo-make provide a different value.

cargo-make should provide the options to script tasks imho. So in addition to options like script_runner and script_extension I'm suggesting you add capture_input and capture_output which will each accept "null", "pipe", or "inherit". If nothing is supplied nothing is passed and run_script use its defaults (which are unchanged from before - input is inherit and output is pipe)

3. capture output shouldn't control input. its hacky.

I agree - not sure where you're seeing that?

[dakom]

Since the options here would be specific to scripts, better names for those might be script_capture_input and script_capture_output

[sagiegurari]

2. this seem too verbose. wish there was a way to support both. that would be best solution. users shouldn't care about that.
   maybe we can pass signals to child process? not sure....
   i think this one needs more investigation

3. you are right. my mistake.

[dakom]

I agree that this PR is not the ultimate end goal, though it does make it more flexible and lets cargo-make have more decision...

On the cargo make side, it doesn't feel that verbose to me but obviously it's your call!

Just to have it on the table, we're talking about most scripts being totally unchanged:

[tasks.input]
script =[
  '''
    echo reading..
    read vv
    echo read $vv
  '''
]

But some edge cases like launching webpack-dev-server would have an extra line or two:

[tasks.webpack-dev-server]
script_capture_input = "null"
script_capture_output = "inherit" # or "pipe"? whatever..
script =[
  '''
    npx webpack-dev-server --config webpack.dev.js
  '''
]

gonna call it a night for now. later!

[dakom]

imho it makes sense to merge this change here, and then the question for how to expose it in cargo-make is a different one

as far as I can see, it's possible to keep cargo-make exactly as it is from the user's perspective even if this change is merged here

(though I do think cargo make should add the script_capture_* stuff as a configurable option - we could open up an issue there to continue the discussion?)

[sagiegurari]

ok agree

[dakom]

cool. not sure how to resolve the codecov stuff... if there's anything I can do to help there let me know

[sagiegurari]

@dakom i'm merging this one. will do few more small changes/tests.
thanks a lot!!!