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.

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

Wrap ./pants help based on actual terminal width #11378

Merged
merged 4 commits into from Mar 5, 2021

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Dec 23, 2020

Rather than assuming 96 columns, we can use shutil to query how wide the user's terminal actually is.

Because we do not memoize this function, it works with terminal resizes. This is crucial when using Pantsd, otherwise the memoization would last the lifetime of the daemon instance.

[ci skip-rust]
[ci skip-build-wheels]

@stuhood
Copy link
Sponsor Member

stuhood commented Mar 3, 2021

@Eric-Arellano : This can likely be refreshed post #11536: I spent some time investigating an issue with widths not propagating, and the Console now deals with "real" file handles. So it should be possible to get them now.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor Author

No luck. Using os.get_terminal_size() results in this with --pantsd:

23:37:09.13 [ERROR] [Errno 25] Inappropriate ioctl for device
Traceback (most recent call last):
  File "/Users/eric/code/pants/src/python/pants/bin/daemon_pants_runner.py", line 134, in single_daemonized_run
    return runner.run(start_time)
  File "/Users/eric/code/pants/src/python/pants/bin/local_pants_runner.py", line 229, in run
    return self._print_help(self.options.help_request)
  File "/Users/eric/code/pants/src/python/pants/bin/local_pants_runner.py", line 210, in _print_help
    return help_printer.print_help()
  File "/Users/eric/code/pants/src/python/pants/help/help_printer.py", line 57, in print_help
    self._print_thing_help()
  File "/Users/eric/code/pants/src/python/pants/help/help_printer.py", line 106, in _print_thing_help
    self._print_options_help(thing, help_request.advanced)
  File "/Users/eric/code/pants/src/python/pants/help/help_printer.py", line 246, in _print_options_help
    formatted_lines = help_formatter.format_options(oshi)
  File "/Users/eric/code/pants/src/python/pants/help/help_formatter.py", line 49, in format_options
    add_option(oshi.basic)
  File "/Users/eric/code/pants/src/python/pants/help/help_formatter.py", line 38, in add_option
    lines.extend(hard_wrap(oshi.description, width=terminal_width()))
  File "/Users/eric/code/pants/src/python/pants/util/memo.py", line 123, in memoize
    result = func(*args, **kwargs)
  File "/Users/eric/code/pants/src/python/pants/util/docutil.py", line 14, in terminal_width
    print(os.get_terminal_size())
OSError: [Errno 25] Inappropriate ioctl for device

shutil.get_terminal_size() calls that function when $COLUMNS isn't defined. Any idea if that's fixable?

@stuhood
Copy link
Sponsor Member

stuhood commented Mar 3, 2021

Ok, interesting. The rust terminal_size crate is able to get it (the UI uses it) using code like this: https://github.com/eminence/terminal-size/blob/68753331337bbf61f19d60511811fc981e67a528/src/unix.rs#L11-L43

Assuming that this is actually a backport: https://github.com/Snaipe/python-rst2ansi/blob/c6f390b45be689a5760060c990e3fe10f502e671/rst2ansi/get_terminal_size.py#L70-L125 ... it's mostly the same code, and the difference appears to be that it's using sys.__stdout__.fileno() rather than sys.stdout.fileno().

The diff below correctly renders:

09:15:51.80 [INFO] terminal_size: os.terminal_size(columns=314, lines=81)
diff --git a/src/python/pants/bin/daemon_pants_runner.py b/src/python/pants/bin/daemon_pants_runner.py
index dff459867..89a0ebf2b 100644
--- a/src/python/pants/bin/daemon_pants_runner.py
+++ b/src/python/pants/bin/daemon_pants_runner.py
@@ -166,6 +166,8 @@ class DaemonPantsRunner(RawFdRunner):
                     stdout_fileno=stdout_fileno,
                     stderr_fileno=stderr_fileno,
                 ), hermetic_environment_as(**env), argv_as((command,) + args):
+                    import shutil
+                    logger.info(f"terminal_size: {shutil.get_terminal_size()}")
                     return self.single_daemonized_run(
                         working_directory.decode(), cancellation_latch
                     )
diff --git a/src/python/pants/init/logging.py b/src/python/pants/init/logging.py
index 1e580f0f5..af8f5e8f3 100644
--- a/src/python/pants/init/logging.py
+++ b/src/python/pants/init/logging.py
@@ -163,10 +163,12 @@ def initialize_stdio(global_bootstrap_options: OptionValueContainer) -> Iterator
             tuple(message_regex_filters),
             log_path,
         )
+        sys.__stdin__, sys.__stdout__, sys.__stderr__ = sys.stdin, sys.stdout, sys.stderr
         # Install a Python logger that will route through the Rust logger.
         with _python_logging_setup(global_level, print_stacktrace):
             yield
     finally:
+        sys.__stdin__, sys.__stdout__, sys.__stderr__ = original_stdin, original_stdout, original_stderr
         sys.stdin, sys.stdout, sys.stderr = original_stdin, original_stdout, original_stderr
 
 

@stuhood
Copy link
Sponsor Member

stuhood commented Mar 3, 2021

Oh, so: alternatively, could use https://docs.python.org/3/library/os.html#os.get_terminal_size ?:

os.get_terminal_size(sys.stdout.fileno())

@stuhood
Copy link
Sponsor Member

stuhood commented Mar 3, 2021

If the sys.__std*__ replacement passes CI, that's a more holistic fix... mind trying that first?

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano changed the title WIP: Wrap ./pants help based on actual terminal width Wrap ./pants help based on actual terminal width Mar 3, 2021
@Eric-Arellano Eric-Arellano marked this pull request as ready for review March 3, 2021 18:45
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -34,7 +35,7 @@ def add_option(ohis, *, category=None):
# No need to repeat those in the advanced section.
title = f"{display_scope} options"
lines.append(self.maybe_green(f"{title}\n{'-' * len(title)}\n"))
lines.extend(hard_wrap(oshi.description))
lines.extend(hard_wrap(oshi.description, width=terminal_width()))
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Each call to terminal_width is a syscall, so it might be good to just call it once at the top.

Comment on lines 171 to 176
sys.__stdin__, sys.__stdout__, sys.__stderr__ = (
original_stdin,
original_stdout,
original_stderr,
)
sys.stdin, sys.stdout, sys.stderr = original_stdin, original_stdout, original_stderr
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Nit: A bit nicer symmetry with the above maybe:

Suggested change
sys.__stdin__, sys.__stdout__, sys.__stderr__ = (
original_stdin,
original_stdout,
original_stderr,
)
sys.stdin, sys.stdout, sys.stderr = original_stdin, original_stdout, original_stderr
sys.stdin, sys.stdout, sys.stderr = original_stdin, original_stdout, original_stderr
sys.__stdin__, sys.__stdout__, sys.__stderr__ = sys.stdin, sys.stdout, sys.stderr

@Eric-Arellano Eric-Arellano merged commit 542dd68 into pantsbuild:master Mar 5, 2021
@Eric-Arellano Eric-Arellano deleted the column-width branch March 5, 2021 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants