SI-7634 resurrect the REPL's :sh command

[gourlaysama]

ProcessResult had a companion object in 2.10 that somehow disappeared in
2.11. It only called "new ProcessResult(...)", so the REPL might just as
well do that.

---

This regression happened because :sh isn't tested; but then, I am not sure how to write a test for this that isn't platform-dependent.

Review by @som-snytt

[som-snytt]

You can test as ReplTest, with code ":sh ..."

Get it? "shello, world."

import scala.tools.partest.ReplTest
import scala.util.Properties.propOrElse

object Test extends ReplTest {
  def java = propOrElse("javacmd", "java")
  def lib  = propOrElse("partest.lib", "scala-library.jar")
  def sep  = propOrElse("path.separator", ":")
  def jarg = s"-classpath $testOutput$sep$lib"
  def code = s""":sh $java $jarg hello.Hello
                |.lines""".stripMargin
}

package hello {
  object Hello extends App {
    Console println "shello, world."
  }
}

and

+Type in expressions to have them evaluated.
+Type :help for more information.
+
+scala> :sh java -classpath sh-run.obj:/home/apm/projects/edge/build/pack/lib/scala-library.jar hello.Hello
+res0: scala.tools.nsc.interpreter.ProcessResult = `java -classpath sh-run.obj:/home/apm/projects/edge/build/pack/lib/scala-library.jar hello.Hello` (1 lines, exit 0)
+
+scala> .lines
+res1: List[String] = List(shello, world.)
+
+scala> 

[gourlaysama]

Ah, I didn't know about those partest properties. The only missing piece was avoiding embedding an absolute path in the check file. It's updated.

Get it? "shello, world."

That's neat.

[gourlaysama]

BTW, is partest only supposed to work from the root of the repo? It fails miserably when i try to call it from test or test/files/run.

[dragos]

The IDE failure is a miss, the build succeeded, but the job was marked as failed because there were no log files to be archived.

[som-snytt]

Mr Phillips had complained that test/partest stopped working -- personally, I had always used ./partest -- and when I fixed that I presumably broke ./partest, but since no one complained, I didn't have the guts and grit to go back in and check it out. I'll make an effort, because it is an unnecessary annoyance.

[som-snytt]

I forgot about the paths-in-check-file problem (due to past mental trauma on that issue).

The absolute-path filter in partest's Runner will strip the test path prefix (but tokenized on space).

There is also the :filter mechanism for test file header comments. I wonder if something like can be made to work. Because you're right, the relative path depends on cwd. So Runner.runCommand should use Process(args, cwd) where cwd is testRoot or whatever.

If cwd is fixed, then the relative path in the checkfile will always be correct.

I think Mr Phillips went to some lengths to absolutize paths everywhere in partest; but inside the test, this relative path is probably OK. (E.g., can you always relativize a path on a different device, say on Windows?)

The tool script sets partest.javacmd but the Runner sets only javacmd, I see by inspection, untested. That might be an oversight. Is the prefixed property set when the test runs?

[gourlaysama]

Hum, I am not entirely sure that cwd would be fixed even if partest was invoked from somewhere else. Although currently one can't do that anyway.

But the filter trick does avoid us all that madness, see update.

I also reverted to javacmd, you're right, the other isn't set, I was fooled by the script.

[som-snytt]

LGTM Sorry to see the :sh go from the check file. I wish the filter were just an ed command.

[retronym]

This looks like it will break if there is a space in the path. I know that's unlikely, but it does force us to discuss whether and how quoting/escaping is performed for the arguments to :sh.

You could also simplify this a touch by not dependending on scala-library; just hand-roll your main method and use System.out.println.

Finally, we should have a test that shows what happens with :sh gobbledegook. If that is too inconvenient to test in partest, perhaps because of an unstable stack trace, at least provide a transcript of a manual test.

I do sort of wonder whether this feature pays its weight, or whether we should just ask people to use scala.sys.process.

[adriaanm]

Sorry this lingered for so long, @gourlaysama! When addressing reviewer feedback, please ping the reviewer so the final LGTM can be given.

[som-snytt]

To coin a phrase, "Wait, what?"

This is OK w/me, but retronym, taxed as he must be, wondered if :sh pulls its weight in future, "but my :sh quotes aren't parsed correctly (on Windows)" issues.

Somewhere on my todo is, Find interesting use cases for :sh. I bet paulp wishes he'd changed it to :awk.

Really, anything with "shello, world" begs commitment.

[gourlaysama]

@adriaanm: oops, I meant to do that, sorry. Actually, I am wondering if fixing this regression was worth it considering what's below.

@som-snytt:

This is OK w/me, but retronym, taxed as he must be, wondered if :sh pulls its weight in future, "but my :sh quotes aren't parsed correctly (on Windows)" issues.
Somewhere on my todo is, Find interesting use cases for :sh. I bet paulp wishes he'd changed it to :awk.

Actually, it is worse than that: :sh quotes aren't parsed correctly on Windows, Linux, Mac OS X, ... since they aren't parsed at all (SI-7947)!

With that in mind, @retronym convinced me that either:

• :sh has to go away because it is broken (so broken the test for this PR is not even assured to work if the javacmd path contains a space, even on Linux)
• or we need for :sh to actually give its input to the current shell, in a reliable way, on all supported platforms. That looks actually possible (forget about Process(), invoke $SHELL on unix, %COMSPEC% on windows, and then feed the string as standard input). I am looking into that.

If I can't get the latter to work, then I think :sh should just be sacrificed.

[adriaanm]

I figured we can merge this PR now (as it's only one line and introduces a tests that seems reliable), and decide whether to remove :sh independently. I'd vote for removal.