Skip to content

Commit

Permalink
avoid failing on standard streams that are None. (#10041)
Browse files Browse the repository at this point in the history
See https://docs.python.org/3/library/sys.html#sys.__stdin__.

> [!Note]
> Under some conditions stdin, stdout and stderr as well as the original values __stdin__, __stdout__ and __stderr__ can be None. It is usually the case for Windows GUI apps that aren’t connected to a console and Python apps started with pythonw.

This is a best-effort solution. External libraries (eg: `tqdm`) and future code that is unaware of this stuff may still fail.
As a console application, `stdout`/`stderr` being `None` is not really something that we should support in general.
Regarding the failure of the daemon, it has been fixed so that it'll always have a stream attached (if not, it falls back to
/dev/null). Today, this is a problem in daemon only if the main process is run with no streams attached, which is not
something that we support or go on length to support.

It is tricky to write tests for this case, and I could not come up with one. Also decided not to test daemon as it should no longer be a problem.
  • Loading branch information
skshetry authored Oct 19, 2023
1 parent b23ba58 commit 825c823
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 16 deletions.
6 changes: 3 additions & 3 deletions dvc/cli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,9 @@ def main(argv=None): # noqa: C901, PLR0912, PLR0915
# the copied parent's standard file descriptors. If we make any logging
# calls in this state it will cause an exception due to writing to a closed
# file descriptor.
if sys.stderr.closed: # pylint: disable=using-constant-test
if not sys.stderr or sys.stderr.closed: # pylint: disable=using-constant-test
logging.disable()
elif sys.stdout.closed: # pylint: disable=using-constant-test
elif not sys.stdout or sys.stdout.closed: # pylint: disable=using-constant-test
logging.disable(logging.INFO)

args = None
Expand Down Expand Up @@ -199,7 +199,7 @@ def main(argv=None): # noqa: C901, PLR0912, PLR0915

logger.trace(args) # type: ignore[attr-defined]

if not sys.stdout.closed and not args.quiet:
if sys.stdout and not sys.stdout.closed and not args.quiet:
from dvc.ui import ui

ui.enable()
Expand Down
6 changes: 3 additions & 3 deletions dvc/daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,12 @@ def _spawn_posix(

# disconnect from the terminal
fd = os.open(os.devnull, os.O_RDWR)
os.dup2(fd, sys.stdin.fileno())
os.dup2(fd, 0)
os.close(fd)

with open(output_file or os.devnull, "ab") as f:
os.dup2(f.fileno(), sys.stdout.fileno())
os.dup2(f.fileno(), sys.stderr.fileno())
os.dup2(f.fileno(), 1)
os.dup2(f.fileno(), 2)

if platform.system() == "Darwin":
# workaround for MacOS bug
Expand Down
6 changes: 4 additions & 2 deletions dvc/logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,11 @@ def set_loggers_level(level: int = logging.INFO) -> None:


def setup(level: int = logging.INFO, log_colors: bool = True) -> None:
from dvc.utils import isatty

colorama.init()

formatter = ColorFormatter(log_colors=log_colors and sys.stdout.isatty())
formatter = ColorFormatter(log_colors=log_colors and isatty(sys.stdout))

console_info = LoggerHandler(sys.stdout)
console_info.setLevel(logging.INFO)
Expand All @@ -205,7 +207,7 @@ def setup(level: int = logging.INFO, log_colors: bool = True) -> None:

show_traceback = bool(os.environ.get(DVC_SHOW_TRACEBACK))
err_formatter = ColorFormatter(
log_colors=log_colors and sys.stderr.isatty(), show_traceback=show_traceback
log_colors=log_colors and isatty(sys.stderr), show_traceback=show_traceback
)
console_errors = LoggerHandler(sys.stderr)
console_errors.setLevel(logging.WARNING)
Expand Down
5 changes: 3 additions & 2 deletions dvc/prompt.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
"""Manages user prompts."""

import logging
import sys
from getpass import getpass
from typing import Collection, Optional

logger = logging.getLogger(__name__)


def ask(prompt: str, limited_to: Optional[Collection[str]] = None):
if not sys.stdout.isatty():
from dvc.ui import Console

if not Console.isatty():
return None

while True:
Expand Down
7 changes: 5 additions & 2 deletions dvc/ui/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,13 @@ def table(
def status(self, status: str, **kwargs: Any) -> "Status":
return self.error_console.status(status, **kwargs)

def isatty(self) -> bool:
@staticmethod
def isatty() -> bool:
import sys

return sys.stdout.isatty()
from dvc import utils

return utils.isatty(sys.stdout)

def open_browser(self, file: "StrPath") -> int:
import webbrowser
Expand Down
5 changes: 3 additions & 2 deletions dvc/ui/pager.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import logging
import os
import pydoc
import sys

from rich.pager import Pager

Expand Down Expand Up @@ -51,7 +50,9 @@ def _pager(text):


def find_pager():
if not sys.stdout.isatty():
from . import Console

if not Console.isatty():
return None

# pylint: disable=redefined-outer-name
Expand Down
3 changes: 1 addition & 2 deletions dvc/updater.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import logging
import os
import sys
import time
from typing import TYPE_CHECKING, Optional

Expand Down Expand Up @@ -123,7 +122,7 @@ def _get_latest_version(self):
def _notify(self, latest: str, pkg: Optional[str] = PKG) -> None:
from dvc.ui import ui

if not sys.stdout.isatty():
if not ui.isatty():
return

message = self._get_message(latest, pkg=pkg)
Expand Down
8 changes: 8 additions & 0 deletions dvc/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import colorama

if TYPE_CHECKING:
from typing import TextIO

from dvc.fs import FileSystem

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -424,3 +426,9 @@ def expand_paths(fs: "FileSystem", paths: Iterable[str]) -> Iterator[str]:
yield from fs.find(path)
else:
yield path


def isatty(stream: "Optional[TextIO]") -> bool:
if stream is None:
return False
return stream.isatty()

0 comments on commit 825c823

Please sign in to comment.