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.

Sign up for GitHub

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

Jump to bottom

deprecate spawn(cmd) to run(cmd, wait=false) #26130

Merged
merged 1 commit into from
Mar 2, 2018
Merged

deprecate spawn(cmd) to run(cmd, wait=false) #26130

merged 1 commit into from
Mar 2, 2018

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Feb 20, 2018 •
edited
Loading

Also add read and write keyword arguments to open for processes.

fixes #25965

@ararslan ararslan added io Involving the I/O subsystem: libuv, read, write, etc. deprecation This change introduces or involves a deprecation labels Feb 20, 2018
@JeffBezanson JeffBezanson force-pushed the jb/runspawn branch 2 times, most recently from 61bc0ef to 4892618 Compare February 20, 2018 22:21
StefanKarpinski
StefanKarpinski approved these changes Feb 20, 2018
View reviewed changes
Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

I thought I was going to be annoyed by run returning the process object when called with wait=true but I'm actually not – I like that it returns the process object. It lets people see that there is a process object wrapping a command with a return code, all of which is good for people to be aware of.

vtjnash
vtjnash approved these changes Feb 24, 2018
View reviewed changes
base/process.jl Outdated

function eachline(cmd::AbstractCmd; chomp=nothing, keep::Bool=false)
if chomp !== nothing
keep = !chomp
depwarn("The `chomp=$chomp` argument to `eachline` is deprecated in favor of `keep=$keep`.", :eachline)
end
stdout = Pipe()
processes = spawn(cmd, (DevNull,stdout,STDERR))
processes = _spawn(cmd, (DevNull,stdout,STDERR))
Copy link
Member

Choose a reason for hiding this comment

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

Are we now unable to open a process without reaching into the internal API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Is there a case not covered by open and pipeline?

@Keno Keno added this to the 1.0 milestone Feb 25, 2018
@o314
Copy link
Contributor

o314 commented Feb 25, 2018

Is there any reason to not use a positive and direct form like run(cmd, async=true)

rather than a double negation run(cmd, wait=false) ?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Feb 26, 2018 •
edited
Loading

"Don't wait" seems more obvious to me than "do run asynchronously". Similarly, "do wait" is significantly clearer than "don't run asynchronously" which is a double negative form of "run synchronously" which I still have to translate in my head to "do wait".

@o314
Copy link
Contributor

o314 commented Feb 26, 2018

I think about parallelism in terms of motion and lanes.
async emphasis both time and lane aspect (async ~ aside). wait tend to flatten both concepts.
So i prefer to use async. That allows me to keep in mind a graph of the whole distributed process.

I stop using mental representation in natural languages very shortly. So no need to escape the hell of understanding things like "don't run asynchronously". that's sound too much convoluted and error-prone for me.

my habits when talking about distributed processing is : use symbols that match to graph as closer as possible. Stick with them as long as possible.

@JeffBezanson
Copy link
Member Author

JeffBezanson commented Feb 26, 2018 •
edited
Loading

I agree with Stefan here. While wait = false might kind-of, sort-of sound like a double negation, async = false is a direct, obvious double negation. It literally says "not-synchronous = false".

In terms of how asynchronous programming is normally described and implemented, the two primitive operations are "run asynchronously" and "wait", not "run asynchronously" and "run synchronously".

@JeffBezanson JeffBezanson force-pushed the jb/runspawn branch 2 times, most recently from b65d1ba to 78037ef Compare February 27, 2018 18:24
@JeffBezanson
Copy link
Member Author

Hmm, this is printing an extra hello in the spawn test. Will investigate.

@JeffBezanson
deprecate spawn(cmd) to run(cmd, wait=false)
1f8f54f 
add `read` and `write` keyword arguments to `open` for processes

fixes #25965
@JeffBezanson
Copy link
Member Author

pkg tests seem to have network issues on OS X sometimes.

@JeffBezanson JeffBezanson merged commit ea62c00 into master Mar 2, 2018
@JeffBezanson JeffBezanson deleted the jb/runspawn branch March 2, 2018 00:17
mbauman added a commit that referenced this pull request Mar 5, 2018
@mbauman
Merge remote-tracking branch 'origin/master' into mb/deprecatemultiva…
95a78df 
…luenonscalarindexedassignment

* origin/master: (28 commits)
  fix an optimizer bug in `invoke` with non-constant functions (#26301)
  lower top-level statements in such a way that the front-end knows (#26304)
  Make sure Sockets page has h1 header (#26315)
  fix doctests, and make them less prone to errors (#26275)
  FIx intro to manual chapter on types (#26312)
  Add a missing "that" (#26313)
  fix docstring for code_llvm (#26266)
  Remove the examples/ folder (#26153)
  download cert.pem from deps-getall, fixes #24903 (#25344)
  Slight update to doc string for at-enum to refer to instances (#26208)
  performance tweak in reverse(::String) (#26300)
  remove references to `TCPSocket` in Base docstrings (#26298)
  Deprecate adding integers to CartesianIndex (#26284)
  Deprecate conj(::Any), add real(::Missing) and imag(::Missing) (#26288)
  fix #26267, regression in `names` with `imported=false` (#26293)
  fix #25857, `typeinfo` problem in showing arrays with abstract tuple types (#26289)
  Add adjoint(::Missing) method (#25502)
  Use lowered form in logging macro (#26291)
  deprecate bin, oct, dec, hex, and base in favor of `string` and keyword args (#25804)
  deprecate `spawn(cmd)` to `run(cmd, wait=false)` (#26130)
  ...
@ararslan ararslan added the breaking This change will break code label Mar 7, 2018
@ararslan
Copy link
Member

ararslan commented Mar 7, 2018

This broke CoverageBase, which calls spawn(cmd, DevNull, STDOUT, STDERR).

@JeffBezanson
Copy link
Member Author

JeffBezanson commented Mar 7, 2018 •
edited
Loading

¯\_(ツ)_/¯

@ararslan ararslan mentioned this pull request Mar 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
breaking This change will break code deprecation This change introduces or involves a deprecation io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.

Rename spawn function
6 participants
@JeffBezanson @o314 @StefanKarpinski @ararslan @vtjnash @Keno