Implement runBackgroundInherit / runMainBackgroundInherit

[swaldman]

For what it's worth, here's a sketch of one possible way to address where runBackground process output should go.

I'm not at all wedded to it! Mostly, since I'm coming at you with asks, I want to do my part to help. For example, @lolgab's suggestion of overridable

def runBackgroundStdout: T[ProcessOutput] = os.Pipe
def runBackgroundStderr: T[ProcessOutput] = os.Pipe

strikes me as perhaps more elegant and flexible than duplicating runBackground commands, which I've done here.

In general, this pull request

1. factors out the common implementations of JavaModule.runBackground and JavaModule.runMainBackground into a helper method.

2. creates variants of Jvm.spawnSubprocess and Jvm.runSubprocess that permit specifying out and err via an optional Tuple2 of ProcessOutput (and reimplements the existing methods, which offer a Boolean flag background, in terms of these methods)

3. modifies the common doRunBackground implementation to accept that optional tuple of ProcessOutput

4. modifies JavaModule.runBackground and JavaModule.runMainBackground to call the new methods, using T.dest/stdout.log and T.dest/stderr.log as outputs

5. adds two new commands JavaModule.runBackgroundInherit and JavaModule.runMainBackgroundInherit which sets both out and err to os.Inherit

Please note the bit in bold. In general, everything old is preserved. No method signatures are removed or changed, there are only additions and reimplementations in terms of those additions. But, following the discussion on discord, I did make one change to the current behavior, dropping the default logs in runBackground.dest / runMainBackground.dest rather than in the project root directory.

I've had to bring in #2752 so that I could give this a try. It seems like it works!

I apologize for the amateurishness. I feel like I'm groping my way around in mill. It was very helpful to learn from @lihaoyi on the discord that ./mill -i installLocal generates a binary one can play with in target/

Thank you all for your work. I love this tool.

[lihaoyi]

@swaldman thanks for the PR!

I think the suggestion of making it a flag on the JavaModule, rather than duplicating the runBackground commands, is the right thing to do. Perhaps def runBackgroundLogPath: T[Option[os.Path]], since os.ProcessOutput isn't generally serializable. If anyone wants a more advanced use case, they can always call the helper methods themselves, so we don't need to complicate the public API for the common simple use case. And I expect that "log to disk here" or "log to stdout" would satisfy 99% of scenarios

[swaldman]

@lihaoyi I agree that sounds better! Proliferating a menagerie of runBackgroundXXX tasks seems inelegant.

I've pushed a new attempt, which...

1. Eliminates runBackgroundInherit / runMainBackgroundInherit.

2. Changes the visibility of the run-background-task-implementing helper method doRunBackground(...) from private to protected, so that modules can use that helper method to roll their own, using any choice of os.ProcessOutput.

3. Implements, per your suggestion, overridable runBackgroundLogPath tasks that configure the built-in runBackground tasks.

   However, I've modified your suggestion in two ways:

   1. Instead of a single runBackgroundLogPath task, I've bifurcated into

      • runBackgroundLogOutPath
      • runBackgroundLogErrPath

      on the theory that sending output and error to different places is not such an unusual case,
      so maybe should be supported.

   2. Instead of returning T[Option[os.Path]], they return T[Option[os.FilePath]]

The reason for that last change was an annoyance defining paths within runBackgroundLogOutPath / runBackgroundLogErrPath.

A common intention might be to log these streams to files in e.g. out/foo/runBackground.dest. But the usual source of an os.Path within runBackgroundLogOutPath -- T.dest -- would place files into out/foo/runBackgroundLogOutPath.dest, which seems less desirable.

So the current implementation

1. sends a stream to os.Inherit by default -- usually the mill console -- when the returned log path is None.
2. sends the stream to whatever absolute path is specified, if Some[os.Path] is returned
3. if Some[os.RelPath] or Some[os.SubPath] is returned, those are interpreted
   relative to the destination director of the runBackground or runMainBackground task.

So if Some(os.sub/"out.txt") is returned by runBackgroundLogOutPath, and runBackground is executed, stdout ends up in out/foo/runBackground.dest/out.txt. This is probably where you want it, rather than Some(T.dest/"out.txt").

This implementation does everything one might want, I think, but it's a judgment call whether it's worth its complexity. Will users understand this well enough to use it, or will it just not be a big deal to return Some(T.dest/"out.txt") and find stdout in out/foo/runBackgroundLogOutPath.dest/out.txt?

Implementing this required adding to JsonFormatters codecs for os.RelPath, os.SubPath, and os.FilePath.
I think it's all okay, the codecs are all mutually compatible in the sense that a thing pickled as an os.Path then
unpickled as an os.FilePath will result in the expected os.Path, etc. (os.RelPath might sometimes canonicalize
to os.SubPath, but only where os.FilePath or os.SubPath would be acceptable.)

Nevertheless it's maybe more uncertainty than my very niche concern over runBackground logging merits.

We could certainly fallback to returning os.Path, and do without the new ReadWriter implicits.

Alternatively, perhaps most parsimoniously, we could forget about the overridable configuration methods entirely, choose whatever as the default behavior for runBackground / runMainBackground, and just leave the doRunBackground(...) helper method exposed for use by submodules. People with a preference for where the streams go can then pretty trivially just implement their own run background tasks that behave however they want.

Sorry to go on for so long!

As always, the intent is to contribute back, not create headaches. I'm not offended if this is all not worth the cognitive load. Thanks again for everything!

[swaldman]

This PR has grown IMHO too complicated and unwieldy. Replaced by #2792