From 7535dcc39dea15d27a115ac1d3c9a787cff79289 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Bertin?= Date: Sat, 17 Aug 2024 21:03:10 +0200 Subject: [PATCH] Give the user some control over progressbar invasiveness Progress reporting is useful but also has its drawbacks, like writing to the calling terminal when it's not in the appropriate state, or slowing down the process being reported on. This commit addresses both situations: 1. a set of 3 configuration variables is introduced with which the user can control (via macports.conf) after how long the progressbar should appear, if it should also show indeterminate progress (aka "we're busy") and what operations should report progress in addition to the traditional ones (fetch, build, destroot). The defaults for these are the current hardwired settings. The "also_for" setting is designed to be a list that currently only accepts "de/activation" for the recent progress reporting added to `port activate` and `port deactivate`. (Which causes a significant increase in duration of those operations.) 2. a Pextlib function is introduced to determine whether `port` runs as a foreground process connected to a terminal. This function is called to check the current state before doing any actual progressbar output. It thus becomes possible to push a long-running operation (say, a build) to the background and do other work in the same terminal, without getting frequent terminal and most likely illegible output. --- doc/macports.conf.in | 14 ++++++++++++++ src/macports1.0/macports.tcl | 15 +++++++++++++++ src/pextlib1.0/Pextlib.c | 36 +++++++++++++++++++++++++++++++++++ src/port/port.tcl | 34 +++++++++++++++++++++------------ src/port1.0/portprogress.tcl | 12 +++++++++--- src/registry2.0/portimage.tcl | 27 ++++++++++++++++++++++++-- 6 files changed, 121 insertions(+), 17 deletions(-) diff --git a/doc/macports.conf.in b/doc/macports.conf.in index 590b7414c0..4555f092ef 100644 --- a/doc/macports.conf.in +++ b/doc/macports.conf.in @@ -136,6 +136,20 @@ variants_conf @MPCONFIGDIR_EXPANDED@/variants.conf # Keep logs after successful installations. #keeplogs no +# Progress reporting. +# MacPorts will show a progressbar during lengthy operations which take more +# than $progressbar_after milliseconds. The bar reports determinate progress +# when the total amount of work and the work done are quantified. It can also +# show an "indeterminate" indication of work being done (and switch to this +# display after a certain amount of time) if $progressbar_type is set to `both`. +# The operations that use this feature are downloads, builds and rev-upgrade, +# plus a list of additional operations $progressbar_also_for which can include +# `de/activation` (for `port deactivate` and `port activate`) and possibly more +# in the future. NB: no quotes for the string values! +# progressbar_after 500 +# progressbar_type both +# progressbar_also_for de/activation + # URLs that MacPorts attempts to download to find out whether a new version was # released. Multiple values, space-separated; only one of the URLs needs to be # available. Downloads will be attempted in the specified order. diff --git a/src/macports1.0/macports.tcl b/src/macports1.0/macports.tcl index e324c2ee03..2ba15f2b9b 100644 --- a/src/macports1.0/macports.tcl +++ b/src/macports1.0/macports.tcl @@ -52,6 +52,7 @@ namespace eval macports { foreach opt [list binpath auto_path extra_env portdbformat \ portarchivetype portimage_mode hfscompression portautoclean \ porttrace portverbose keeplogs destroot_umask release_urls release_version_urls \ + progressbar_after progressbar_type progressbar_also_for \ rsync_server rsync_options rsync_dir \ startupitem_autostart startupitem_type startupitem_install \ place_worksymlink xcodeversion xcodebuildcmd xcodecltversion xcode_license_unaccepted \ @@ -78,6 +79,7 @@ namespace eval macports { registry.path registry.format user_home user_path user_ssh_auth_sock \ portarchivetype portarchive_hfscompression archivefetch_pubkeys \ portautoclean portimage_mode porttrace keeplogs portverbose destroot_umask \ + progressbar_after progressbar_type progressbar_also_for \ rsync_server rsync_options rsync_dir startupitem_autostart startupitem_type startupitem_install \ place_worksymlink macportsuser sudo_user \ configureccache ccache_dir ccache_size configuredistcc configurepipe buildnicevalue buildmakejobs \ @@ -983,6 +985,9 @@ proc mportinit {{up_ui_options {}} {up_options {}} {up_variations {}}} { macports::cxx_stdlib \ macports::hfscompression \ macports::portarchive_hfscompression \ + macports::progressbar_after \ + macports::progressbar_type \ + macports::progressbar_also_for \ macports::host_cache \ macports::porturl_prefix_map \ macports::ui_options \ @@ -1460,6 +1465,16 @@ Please edit sources.conf and change '$url' to '[string range $url 0 26]macports/ } set portarchive_hfscompression $hfscompression + if {![info exists progressbar_after]} { + set progressbar_after 500 + } + if {![info exists progressbar_type]} { + set progressbar_type "both" + } + if {![info exists progressbar_also_for]} { + set progressbar_also_for "de/activation" + } + # Set rync options if {![info exists rsync_server]} { set rsync_server rsync.macports.org diff --git a/src/pextlib1.0/Pextlib.c b/src/pextlib1.0/Pextlib.c index 4d0ccadccb..251dc9a915 100644 --- a/src/pextlib1.0/Pextlib.c +++ b/src/pextlib1.0/Pextlib.c @@ -1150,6 +1150,40 @@ int ClonefileCmd(ClientData clientData UNUSED, Tcl_Interp *interp, int objc, Tcl return TCL_ERROR; } +/* + processIsForeground + + synopsis: processIsForeground + + Returns true if the process is running in the foreground or false + if in the background. +*/ +int IsProcessForegroundCmd(ClientData clientData UNUSED, Tcl_Interp *interp, int objc, Tcl_Obj *CONST objv[]) +{ + /* Check the arg count */ + if (objc != 1) { + Tcl_WrongNumArgs(interp, 1, objv, NULL); + return TCL_ERROR; + } + + int fd, fdo = fileno(stdout); + if ((fd = isatty(fdo) ? fdo : open("/dev/tty", O_RDONLY)) != -1) { + const pid_t pgrp = getpgrp(); + const pid_t tcpgrp = tcgetpgrp(fd); + if (fd != fdo) { + close(fd); + } + if (pgrp != -1 && tcpgrp != -1) { + Tcl_SetObjResult(interp, Tcl_NewBooleanObj(pgrp == tcpgrp)); + return TCL_OK; + } + } + Tcl_SetErrno(errno); + Tcl_ResetResult(interp); + Tcl_AppendResult(interp, "processIsForeground: ", (char *)Tcl_PosixError(interp), NULL); + return TCL_ERROR; +} + int Pextlib_Init(Tcl_Interp *interp) { if (Tcl_InitStubs(interp, "8.4", 0) == NULL) @@ -1214,6 +1248,8 @@ int Pextlib_Init(Tcl_Interp *interp) Tcl_CreateObjCommand(interp, "fs_clone_capable", FSCloneCapableCmd, NULL, NULL); Tcl_CreateObjCommand(interp, "clonefile", ClonefileCmd, NULL, NULL); + Tcl_CreateObjCommand(interp, "processIsForeground", IsProcessForegroundCmd, NULL, NULL); + if (Tcl_PkgProvide(interp, "Pextlib", "1.0") != TCL_OK) return TCL_ERROR; diff --git a/src/port/port.tcl b/src/port/port.tcl index 56b2e27dcc..1018098a03 100755 --- a/src/port/port.tcl +++ b/src/port/port.tcl @@ -4829,9 +4829,11 @@ namespace eval portclient::progress { proc initDelay {} { variable show variable startTime + variable showTimeThreshold set startTime [clock milliseconds] set show no + set showTimeThreshold ${macports::progressbar_after} } ## @@ -4907,7 +4909,9 @@ namespace eval portclient::progress { } update { lassign $args now total - if {[showProgress $now $total] eq "yes"} { + # Check on each update if we're still outputting to a tty - the user can + # have pushed us into the background. + if {[processIsForeground] && ([showProgress $now $total] eq "yes")} { set barPrefix " " set barPrefixLen [string length $barPrefix] if {$total != 0} { @@ -4919,11 +4923,13 @@ namespace eval portclient::progress { } intermission - finish { - # erase to start of line - ::term::ansi::send::esolch stderr - # return cursor to start of line - puts -nonewline stderr "\r" - flush stderr + if {[processIsForeground]} { + # erase to start of line + ::term::ansi::send::esolch stderr + # return cursor to start of line + puts -nonewline stderr "\r" + flush stderr + } } } @@ -4955,7 +4961,9 @@ namespace eval portclient::progress { } update { lassign $args type total now speed - if {[showProgress $now $total] eq "yes"} { + # Check on each update if we're still outputting to a tty - the user can + # have pushed us into the background. + if {[processIsForeground] && ([showProgress $now $total] eq "yes")} { set barPrefix " " set barPrefixLen [string length $barPrefix] if {$total != 0} { @@ -4974,11 +4982,13 @@ namespace eval portclient::progress { } } finish { - # erase to start of line - ::term::ansi::send::esolch stderr - # return cursor to start of line - puts -nonewline stderr "\r" - flush stderr + if {[processIsForeground]} { + # erase to start of line + ::term::ansi::send::esolch stderr + # return cursor to start of line + puts -nonewline stderr "\r" + flush stderr + } } } diff --git a/src/port1.0/portprogress.tcl b/src/port1.0/portprogress.tcl index f0165e73b5..f50f93ec0a 100644 --- a/src/port1.0/portprogress.tcl +++ b/src/port1.0/portprogress.tcl @@ -41,6 +41,7 @@ namespace eval portprogress { variable indeterminate_timer 0 # If our progress callback should issue indeterminate progress updates + variable allow_indeterminate yes variable indeterminate yes # ninja ([/]) @@ -53,7 +54,8 @@ namespace eval portprogress { # A SystemCmd callback that parses common target progress formats to display # a progress bar proc portprogress::target_progress_callback {event} { - global portverbose + global portverbose progressbar_type + variable allow_indeterminate variable indeterminate variable indeterminate_timer variable indeterminate_threshold @@ -66,7 +68,11 @@ proc portprogress::target_progress_callback {event} { switch -- [dict get $event type] { exec { - set indeterminate yes + # reset allow_indeterminate to the user/default-configured setting + if {${progressbar_type} ne "both"} { + set allow_indeterminate no + } + set indeterminate ${allow_indeterminate} set indeterminate_timer 0 ui_progress_generic start } @@ -102,7 +108,7 @@ proc portprogress::target_progress_callback {event} { set time_diff [expr { $time_now - $time_last }] if {${time_diff} >= ${indeterminate_threshold}} { - set indeterminate yes + set indeterminate ${allow_indeterminate} } } diff --git a/src/registry2.0/portimage.tcl b/src/registry2.0/portimage.tcl index 0d6652944b..c5b06fae79 100644 --- a/src/registry2.0/portimage.tcl +++ b/src/registry2.0/portimage.tcl @@ -63,6 +63,7 @@ namespace eval portimage { variable force 0 variable noexec 0 variable UI_PREFIX {---> } +variable show_progressbar 1 # takes a composite version spec rather than separate version,revision,variants proc activate_composite {name {v ""} {optionslist ""}} { @@ -161,6 +162,7 @@ proc deactivate_composite {name {v ""} {optionslist ""}} { proc deactivate {name {version ""} {revision ""} {variants 0} {options ""}} { variable UI_PREFIX + variable show_progressbar if {[dict exists $options ports_force] && [string is true -strict [dict get $options ports_force]] } { # this not using the namespace variable is correct, since activate @@ -247,6 +249,12 @@ proc deactivate {name {version ""} {revision ""} {variants 0} {options ""}} { } } + if {[lsearch -exact ${macports::progressbar_also_for} "de/activation"] < 0} { + set show_progressbar 0 + # minimise the overhead of calling _progress: it wouldn't do a thing so make it a stub + proc _progress {args} {} + } + ui_msg "$UI_PREFIX [format [msgcat::mc "Deactivating %s @%s"] $name $specifier]" try { @@ -428,6 +436,8 @@ proc _activate_directories {dirs imageroot} { # extract an archive to a directory # returns: path to the extracted directory proc extract_archive_to_imagedir {location} { + variable show_progressbar + set extractdir [file rootname $location] if {[file exists $extractdir]} { set extractdir [mkdtemp ${extractdir}XXXXXXXX] @@ -586,7 +596,11 @@ proc extract_archive_to_imagedir {location} { } else { set cmdstring "${unarchive.pipe_cmd} ( ${unarchive.cmd} ${unarchive.pre_args} ${unarchive.args} )" } - system -callback portimage::_extract_progress $cmdstring + if {${show_progressbar}} { + system -callback portimage::_extract_progress $cmdstring + } else { + system $cmdstring + } } on error {_ eOptions} { ::file delete -force $extractdir throw [dict get $eOptions -errorcode] [dict get $eOptions -errorinfo] @@ -627,7 +641,9 @@ proc _extract_progress {event} { } proc _progress {args} { - if {[macports::ui_isset ports_verbose]} { + variable show_progressbar + + if {!${show_progressbar} || [macports::ui_isset ports_verbose]} { return } @@ -693,6 +709,7 @@ proc _activate_contents {port {rename_list {}}} { variable keep_imagedir variable progress_step variable progress_total_steps + variable show_progressbar set files [list] set baksuffix .mp_[clock seconds] @@ -701,6 +718,12 @@ proc _activate_contents {port {rename_list {}}} { set imagefiles [$port imagefiles] set num_imagefiles [llength $imagefiles] + if {[lsearch -exact ${macports::progressbar_also_for} "de/activation"] < 0} { + set show_progressbar 0 + # minimise the overhead of calling _progress: it wouldn't do a thing so make it a stub + proc _progress {args} {} + } + set progress_step 0 if {[::file isfile $location]} { set progress_total_steps [expr {$num_imagefiles * 3}]