com-lihaoyi / os-lib

OS-Lib is a simple, flexible, high-performance Scala interface to common OS filesystem and subprocess APIs
Other
672 stars 63 forks source link

Add API to redirect `os.Inherit`ed streams globally to be consistent with std streams redirections #283

Closed lihaoyi closed 1 month ago

lihaoyi commented 1 month ago

Fixes https://github.com/com-lihaoyi/os-lib/issues/282

Unlike usage of System.in/System.out/System.err programmatically, or the scala.Console equivalents, output from subprocesses with os.Inherit is not affected by System.setIn/setOut/setErr. This is often counterintuitive, because you expect redirecting these streams to redirect all output, only to find output from subprocess leaking through a side channel to the original process in/out/err.

This PR adds the os.Inherit.in/out/err dynamic variables, which can be used to re-assign os.Inherit on a scoped threadlocal basis. This allows developers who use System.setOut or Console.withOut to also redirect subprocess output within the scope of the stdout redirect, without users working within those scopes to have to remember to redirect them manually at every callsite.

Because we do not control System.setOut or Console.withOut, there isn't really a way for us to detect and do this automatically (e.g. redirects may happen even before OS-Lib has been even classloaded) and so we have to rely on it being opt-in.

As an escape hatch, in case users do not want this behavior, we provide a os.Inherit0 redirect which performs identically to the original os.Inherit even in the process of redirecting os.Inherit.{in,out,err}

Covered by an added unit test

lihaoyi commented 1 month ago

I added a line in the changelog; we can postpone adding it to the main readme body until after it is published to avoid confusion

lefou commented 1 month ago

IMHO, we shouldn't postpone the documentation of a new feature to a subsequent release. It would be missing in the tagged release then and not available for offline reading, for example.

We could add a since <next version> marker to the new doc section, which would be helpful in any case.

lihaoyi commented 1 month ago

Updating it together with tagging/publishing the release would ensure the smallest gap between what's documented and what's published. I don't want people to read docs and have to consciously skip parts that dont apply

It's a compromise, but some compromise is inevitable when most people will see the docs on the web from the latest version of main while releases are done less frequently.

lefou commented 1 month ago

It's your call.


I kind of disagree, though. It's what we did in the past and what I and others do in other projects. The Readme and other documentation should reflect the current state of the branch it belongs to. Looking at some possibly unreleased state is an inherent property of looking at the main branch. In contrast to looking at a tag or release branch. It's a good thing to keep features and their docs close, ideally in the same PR. That enables us to cut a release including its documentation at any point in time.

Users can look at a specific tag rather easily. The docs then always reflects that version.

To provide stable documentation reflecting a specific version, we could and probably should just copy a rendered version to ghpages. That can be automated.