Bug 202058 - bsdinstall - Disable dialog(3) for distfetch/checksum/distextract targets
Summary: bsdinstall - Disable dialog(3) for distfetch/checksum/distextract targets
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-sysinstall
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-08-03 10:31 UTC by Ganael LAPLANCHE
Modified: 2016-11-28 14:44 UTC (History)
4 users (show)

See Also:


Attachments
Patch against -HEAD (9.49 KB, text/plain)
2015-08-03 10:31 UTC, Ganael LAPLANCHE
no flags Details
Patch for -CURRENT (r309088) (9.72 KB, patch)
2016-11-24 14:11 UTC, Ganael LAPLANCHE
no flags Details | Diff
Patch for distextract r309246 (5.53 KB, patch)
2016-11-28 14:43 UTC, Ganael LAPLANCHE
no flags Details | Diff
Patch for distfetch r309246 (4.61 KB, patch)
2016-11-28 14:43 UTC, Ganael LAPLANCHE
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ganael LAPLANCHE 2015-08-03 10:31:32 UTC
Created attachment 159488 [details]
Patch against -HEAD

Hi,

While is it possible to use several installer targets in a fully unattended way, distfetch, checksum and distextract targets always use dialog(3) to display their status.

Find attached a patch proposal to prevent those targets from using dialog(3) if the "nonInteractive" environment variable is set.

This would simplify scripting the installer, e.g. over SSH, where a TTY is not always allocated.
Comment 1 fre.fbsdpr 2015-11-12 15:27:18 UTC
Hi,

I agree, except that a non-camelCase/-internal variable name should be made available.  This would also resolve Bug 203777.

For now, I use a distfetch replacement for unattended installs, but since I use the 'script' target I still get dialog(3), mostly blanking the console screen to show a mouse pointer and a cursor.
Comment 2 Ganael LAPLANCHE 2016-11-24 14:11:07 UTC
Created attachment 177357 [details]
Patch for -CURRENT (r309088)
Comment 3 Ganael LAPLANCHE 2016-11-24 14:11:49 UTC
Hi,

I am not sure to understand your comment. Only the 'distfetch', 'checksum' and 'distextract' targets are fixed by that patch.

I've attached an updated version of the patch (that builds successfully on r309088).

Best regards,

Ganael.
Comment 4 Devin Teske freebsd_committer 2016-11-24 20:24:41 UTC
Apologies for not picking this up off of -sysinstall@ (I'm sure I was CC'd, but I am quite a busy gal).

Using "nonInteractive" to achieve what you want is not historically accurate with the use of the variable in both bsdconfig and sysinstall.

In bsdconfig, you can see its use by executing the following:

$ grep -nrs --color '\(VAR_NONINTERACTIVE\|nonInteractive\)' /usr/share/bsdconfig

And for sysinstall (from base/stable/9/usr.sbin/sysinstall/):

$ grep -Inrs --color '\(VAR_NONINTERACTIVE\|nonInteractive\)' ~/src/freebsd_svn/base/stable/9/usr.sbin/sysinstall

NB: You may have your stable/9 sources elsewhere

And for bsdinstall

grep -Inrs --color '\(VAR_NONINTERACTIVE\|nonInteractive\)' /usr/libexec/bsdinstall


Any grep above should indicate that VAR_NONINTERACTIVE (nonInteractive; configurable in source) doesn't actually disable dialog. It disables prompts for information.

That is to explain:

When nonInteractive is NULL or unset, you can be prompted.
When nonInteractive is set and non-NULL (e.g., 1), you'll get an error if required information is not provided in your script.

NB: sysinstall was essentially split in twain; install bits to bsdinstall, configure bits to bsdconfig.

I'm all-for teaching bsdinstall to use $nonInteractive more often. However, its purpose is not to disable dialog, just disable prompts which can hang up scripts because nobody is there to answer the prompt.

ASIDE: By the way, I'm super impressed that your patch included the use of DPV_DISPLAY_STDOUT ;D (you're thorough, and I like that).

Getting back to the issue at-hand...

I think the appropriate solution is to:

1. Add support for HAVE_DIALOG (see SVN r307802 -- link below)

NB: That will dove-tail nicely into the following work(s)...
https://svnweb.freebsd.org/base?view=revision&revision=307802 [bapt+emaste]
https://svnweb.freebsd.org/base?view=revision&revision=306648 [emaste]
https://svnweb.freebsd.org/base?view=revision&revision=306375 [emaste+dteske]

2. Use printf when HAVE_DIALOG is unset

and optionally...

3. Add a command-line flag that inhibits the use of dialog in the same manner as using WITHOUT_DIALOG to compile-out the HAVE_DIALOG sections.

You can see from the above SVN revisions:

a. First came r306375; we wanted to introduce WITHOUT_DIALOG; that caused us to have to disable bsdinstall, bsdconfig, dpv, and tzsetup

b. However, bapt made tzsetup work without dialog in SVN r307802 paving the way to have it re-enabled permanently

And you've correctly identified that both dpv(3) and dpv(1) support non-dialog interfaces (e.g., stdout via DPV_DISPLAY_STDOUT for dpv(3) and for dpv(1), the `-d' flag).

If, with your help, we can add "#ifdef HAVE_DIALOG" support to the bsdinstall components that you correctly identified as needing it (distfetch, checksum, and distextract) and I help fix all the shell code, we could get these components working in a dialog-less world.

What do you think?
Comment 5 Devin Teske freebsd_committer 2016-11-24 20:34:19 UTC
The way that I have in-mind with how to support WITHOUT_DIALOG for the shell code (which is not compiled, unlike the C code):

a. When WITHOUT_DIALOG is unset and unused, install the normal "dialog.subr" which provides all the f_dialog_*() shell functions, using dialog(1)

b. When WITHOUT_DIALOG is set in src.conf(5) and present when usr.sbin/bsdconfig/Makefile is processed, install an "anti_dialog.subr" (name pending) that provides the same functions but use printf/f_err/etc. to stdout instead of dialog. That way we don't have to go changing massive amounts of shell code and all the f_dialog_msgbox/f_show_msg/etc. invocations will still work as-expected.
Comment 6 Ganael LAPLANCHE 2016-11-25 10:02:30 UTC
Hi Devin,

Thank you very much for your answer.

There may be a misunderstanding (the title of my PR is propably inaccurate): the goal of my patch is not to disable the need for dialog(3) at build time. My initial -exact- need is to be able to run the tools over a SSH session without a tty, but *without having to recompile the tools with a specific option*, so we are mostly dealing with you suggestion 3 here. Making the tools build WITHOUT_DIALOG is (in my opinion) a step further that can be achieved later.

Anyway, you're right, the 'nonInteractive' env variable is not appropriate everywhere in my patches: sometimes it is used to prevent dialog_msgbox(,,,,TRUE) from waiting for "OK" to be pressed after an error message and sometimes it is also used to disable infoboxes or general dialog(3) code. In the first case, I think it is OK, as pressing "OK" is an interactive action. In the second case, we should probably introduce a way to disable dialog(3) and use printf() instead.

So the patch should probably be re-written to split the modifications in two kinds:

- the ones that really disable interactivity (dialog_msgbox(,,,,TRUE)) => can be left as is IMHO
- the ones that really only deal with dialog(3) stuff => to be reworked. For that purpose, we could introduce a new env variable ($noDialog ?) to keep the global spirit of the tools.

Finaly, there is probably a relationship between the two variables :

/* ---- */

static uint8_t interactive = 1;
static uint8_t dialog = 1;

if (getenv("noDialog") != NULL) {
    interactive = 0;
    dialog = 0;
}

if (getenv("nonInteractive") != NULL) {
    interactive = 0;
}

/* ---- */

Does that seem reasonable to you ?

For the 'checksum' script, we could do the same kinds of modifications. E.g., for the second chunk :
 
+   if f_dialog; then
+       dialog --backtitle "FreeBSD Installer" --title "Checksum Verification" \
+           --mixedgauge "Verifying checksums of selected distributions." \
+           0 0 $percentage $items
+   else
+       printf "Verifying checksum of %s\n" "$dist"
+   fi
 
 The third and last chunk should probably be rewritten that way :

+       if f_interactive; then
+           dialog --backtitle "FreeBSD Installer" --title "Error" \
+               --msgbox "The checksum for $dist does not match. It may have become corrupted, and should be redownloaded." 0 0
+       else
+           if f_dialog; then
+               dialog --backtitle "FreeBSD Installer" --title "Error" \
+                   --infobox "The checksum for $dist does not match. It may have become corrupted, and should be redownloaded." 0 0
+           else
+               f_err "The checksum for %s does not match. It may have become corrupted, and should be redownloaded.\n" "$dist"
+           fi
+       fi

Not sure if f_dialog is well-chosen but you get the idea...

Regards,
Ganael.
Comment 7 Devin Teske freebsd_committer 2016-11-25 22:49:59 UTC
I recommend the following:

For C code, we should be using isatty(3) on stdout (fd=1) to determine whether we use dialog(3) vs printf(3).

For shell code, we should be using stty(1) to determine whether we use dialog(1) vs f_err()/printf(1).

It doesn't make sense to use dialog(1,3) when stdout is not a TTY, so the code should dynamically detect this situation and adjust accordingly (no need for setting a variable).

For distfetch, distextract, and cksum, your printf's are good-to-go, but the conditional should be something like "if (notty)" which would be set when isatty(3) tells us that fd=1 is not a TTY.

For bsdconfig, I would like to teach /usr/share/bsdconfig/dialog.subr to test for TTY on include (e.g., ``stty >&- 2>&- || setvar $VAR_NOTTY 1''*) and if set, dialog.subr would use f_err()/printf(1) instead of dialog(1). Then all the other code could rely on f_dialog_* without having to address each invocation.

It would be rather slick if we allowed the environment to override the default interpretation. That is to explain:

If fd=1 is not a TTY (in C) or stty(1) reports error (in sh), default to using printf(1,3). An environment variable (I suggest $noTty, $VAR_NOTTY), if set and non-NULL overrides the default setting. In fact, if set, don't even perform the isatty(3)/stty(1) test.


* in /usr/share/bsdconfig/variable.subr we would add a "f_variable_new VAR_NOTTY noTty"
Comment 8 Devin Teske freebsd_committer 2016-11-25 22:55:23 UTC
I still need to make sure that we build upon bapt, emaste, and my own work thus far with respect to the following (previously mentioned) work:

https://svnweb.freebsd.org/base?view=revision&revision=307802 [bapt+emaste]
https://svnweb.freebsd.org/base?view=revision&revision=306648 [emaste]
https://svnweb.freebsd.org/base?view=revision&revision=306375 [emaste+dteske]

I had previously suggested that a good solid approach would be to first and foremost follow the approach of bapt with respect to tzsetup, and then optionally add a command-line flag to each of the applications to enable runtime disabling of dialog(1).

I do like to think of iterative approaches that build upon and reinforce prior works.

The shortcomings of distfetch, distextract, and cksum are also solved by way of fixing the other issue of them not being able to run in a dialog(1,3)-less environment -- regardless of that being a decision that is made at compile-time or runtime.

I think the best approach would be to use isatty(3)/stty(1) at runtime to invoke the same code-path as WITHOUT_DIALOG.
Comment 9 Ganael LAPLANCHE 2016-11-28 14:43:08 UTC
Created attachment 177485 [details]
Patch for distextract r309246
Comment 10 Ganael LAPLANCHE 2016-11-28 14:43:37 UTC
Created attachment 177486 [details]
Patch for distfetch r309246
Comment 11 Ganael LAPLANCHE 2016-11-28 14:44:01 UTC
Hi Devin,

Thanks for your answer. You're definitely right, tty availability should be detected at runtime.

I have focused on C tools (i.e. distfetch and distextract). Find attached two patches -one for each tool- implementing what has been written so far.

A few things may still be addressed:

- libdpv(3) is not built at all if dialog(3) is not built, anyway it can be used with DPV_DISPLAY_STDOUT which does not need dialog(3). Maybe HAVE_DIALOG stuff can be added there too to enable building libdpv(3) without dialog(3) ?
- output of distextract is not really sexy when using DPV_DISPLAY_STDOUT (anyway, "it works (TM)")
- other code from bsdinstall still needs work : partedit (does that make sense ?) and scripts/ directory
- we may still want to add nonInteractive stuff, because a few calls to dialog_msgbox(,,,,TRUE) remain interactive, just waiting for the user to acknowledge the msg box
- finally, I have not added any option to the tools and rely on the 'noTty' env variable only (to keep the original spirit of the tools)

Best regards,
Ganael.