Since I just spent some time whacking on code near some of the things
you metion, I think i might have some useful input...  8-)

Jonathan Stone <jonathan@DSG.Stanford.EDU> writes:
> I've patched my local tree to use named variants for the 
> common cases to run_prog as follows:
> 
>         run_prog(0, 0, NULL, ...) -> run_silent(...);
>         run_prog(1, 0, NULL, ...) -> run_prog_or_die(...);
>         run_prog(0, 1, NULL, ...) -> run_prog_in_subwin(...);
> 
> joined the command lines to make them more readable.

Cool!

> run_prog() is
> replacd with a va_run_prog(), taking a va_alist as last arg, and all
> the run_* variants coded as wrappers to va_run_prog.

One of these things is not like the others...  8-)

It might be nice to have it named run_<something>, e.g. run_va.  it's
nice to be able to look for functions named 'run_.*'


> One thing it did
> highlight is that run_prog_or_die() simply calls exit. It needs to
> display a message saying what failed; not doing so is a bug,
> and we should really fix that for 1.4.1.

I'm kinda hesitant to buy into additional significant changes for
1.4.1's sysinst, at least until it gets some real user testing.

In what cases is run_prog_or_die() used (or the 'fatal' run_prog()
flag currently used)?  It seems to me that, in general, dying is a
very bad thing, and should be avoided.

For this (and the silent case), it probably makes sense to accumulate
error messages off-screen and, if a failure occurs, display them in a
message window like the run-w/display window.

In the longer term, sysinst's "exit strategy" needs some cleanup, in
my opinion, as does its data/state storage strategy.  It'd be nice to
just have the cleanup hook print, among other things, why the program
was dying.  However, it's not clear to me that 


> I've done a few more random cleanups: e.g., moving the script/log
> processing inside do_system() rather than dupliating it at each call
> to do_system(), and adding a target_rm() function rather than, e.g.,
> 
>       run_silent("rm -f %s", target_expand("/.profile"));
> 
> since target_expand() was supposed to be private to target.c.

Cool.


> I've also looked at the logging and scriting functionality.  Both are
> great ideas but the implementation leaves something to be
> eseired. [ ... ]
> 
> I'd like to  add some functions to do log/script processing
> via a `FILE*/script/log handle, e.g.,

It seems to me that it's reasonable to assume in that code that the
script and log handles are globals.  (not clear if you meant that from
your statement above.)

I dunno, it might also be nice to have functions which stuff bits into
the log/script (one thing that jumps out at me is _comments_, so
somebody reading the thing can know what's going on!) if they're
enabled so that you don't need to even check for logging/scripting
being enabled.  They could be used by your proposed target_*()
functions, even.


> Comments?  Anyone like to review these before a commit?

If you'd like, and are willing to wait a little bit for a response (no
earlier than this weekend, I think), I'd be willing to take a gander
at them.


> Or should we
> give up on the sysinst codebase as a dead duck?

No.

While some people have indicated that they're working on Something
Better, it doesn't sound like they're particularly far along (or even
have a rough estimate of completion).  Therefore, as far as i'm
concerned, we're stuck with sysinst for the forseeable future.

I have a bunch of work planned for it, if I can make the time.  I
think the list is something like the following (in order):

[ in the entire discussion below, when i'm referring to menus i'm
referring to the things sysinst calls menus, which are really more
like selection boxes or something. ]

	1 convert the msg code to do automatic layout of text.  This
	  means several things:

		a add APIs to provide table formatting, i.e. no
		  auto-flow of text, and convert table formatting code
		  to use it.

		b turn on the simple layout engine that i committed
		  a week or two ago.  Right now this isn't enabled
		  because of the lossage with tables.

		c nuke unused APIs.

		d cleanup markup features for messages, e.g. "force
		  line break, force paragraph break."  Right now this
		  functionality is already in the layout engine, but
		  it's... kinda hokey since I wanted it to be mostly-
		  compatible with the existing message text.  Right
		  now, paragraphs are denoted by \n\n, and line breaks
		  are denoted by <space>\n.  If possible I'd like to
		  do these in an HTML compatible way, but I'll cross
		  that bridge when I come to it.  In general, this
		  item can be put off to much later.

	2 poke the message compiler so that messages have a couple of
	  attributes: "preformatted" (don't auto-flow, i.e. preserve
	  spacing), and "menu selection character."  The former would
	  be used for table headings and table body entries.

	3 nuke all native-language (i.e. not command names, etc., but
	  "user feedback" text) and table format string constants in
	  the code.  (You'd think with a message compiler there
	  wouldn't be any of the former, but you'd be wrong.  I made
	  the problem a tiny bit worse with my recent changes, but
	  there were hard-coded strings where it would have been easy
	  to DTRT already...).  Replace them with messages.

	4 nuke all public message APIs which operate directly on
	  strings.  Make them all require messages.

	5 make the menu system suck much, much less.  make all menus
	  dynamic, and capable of having entries changed, added, and
	  removed at run time.  What this really means is:

		a create a decent menu API.  Knowing little, I was
		  planning to grab one of my old copies of Inside
		  Macintosh and clone some of the basic menu
		  creation/deletion + item addition/removal APIs.
		  They worked pretty well for me when I had to use
		  them 10+ years ago.  8-)  Desired features:

			* create, destroy menus

			* add, remove menu items

			* change item text.

			* each item can have a predefined keyboard
			  shortcut, but failing that keyboard
			  shortcuts will be assigned as it's done
			  currently.

			* settable default item.

			* menu text items can be described by message
			  IDs.

		b convert the menu compiler to generate an init
		  function which generates calls to that API, rather
		  than hard-coding the tables.

		c make the menu compiler try to do basic menu size vs.
		  screen size sanity checks at startup, rather than as
		  menus are invoked.  No more exiting out in the
		  middle because there's not enough room for the menu!

		d depending on the space cost incurred by 5b, and the
		  pain involved in generating static entries for the
		  preconfigured menus, try to generate pregenerated
		  menu structures rather than just function calls.
		  This is an optimization, it can be deferred.

		[ note that this means that the menu system will
		  probably be using the message system's internal
		  APIs.  I don't consider this a bug; i consider the
		  two tightly intertwined. ]		
		
	6 move all of the menu message strings into the message files,
	  and nuke the language dependence in the menu files.  It's
	  just Evil to be mixing code and data like this.  A
	  maintenance nightmare, as I think most of us who've hacked
	  the code know by now.  8-)

	7 nuke all handling of fixed strings in the menu code, even
	  in the 'replace item text' code.  (There you can do
	  formatting based on messages.)  Require that all public APIs
	  operate on messages.

	8 Integrate message window and menu display, or at least put
	  in some status checking features so that the menus better
	  accomodate the size of the message in the message window.
	  Maybe glue them together even more tightly and require that
	  the message window be the entire stdscr.


Pie in the sky:

	* more advanced message formatting.  e.g. don't make the
	  message formatter just printf-like, make it support
	  additional types, such as messages.  For instance, it'd be
	  nice to say:

		message mi_foobar "foo bar: %m"
		message mi_foobar_placeholder "XXX"
		message mi_foobar_yes "Yes"
		message mi_foobar_no "No"

	  then msg_format(MSG_mi_foobar, MSG_mi_placeholder); to get
	  "foo bar: XXX".  (the reason for using a placeholder here
	  is so that the initial menu size check could try to
	  check for enough horizontal space to display the menu.  I
	  prefer that style to hard-coding one of the actual message
	  values, because e.g. the widest one may change from language
	  to language.)


Other code that I know needs work:

	* the code that reads the child's output from the pty and then
	  logs it and/or puts it into the display window is losing,
	  for a couple of reasons:

		* it does: parent_fork()->child1
		  child1_fork()->child2, child1 exec command, child2
		  read output and feed it back to parent.  This
		  seems...  unnecessarily complex.  If there's a
		  reason for it, it should be thorougly documented.
		  If not, it should be cleaned up to do only one fork.

		* if the command exits before all output has been
		  passed back to the parent, the parent won't ever
		  get (and therefore display/log) the last of the
		  output.  This is caused by incorrect loop
		  termination checks, but (given my experience) I
		  don't know enough that it's trivial to fix so I
		  punted on it for the time being.  I seem to recall
		  that running 'stty -a ; false' in a subwindow was
		  a good way to show the problem.  you might not need
		  the '; false' to trigger it, but if you have it
		  the window doesn't go away immediately and you can
		  see the last output pushed to the parent.  8-)

	* I've seen several cases now where (it would appear that)
	  running commands w/o display output has caused sysinst to
	  hang.  This could be because of some child-exit race
	  condition check, or something.  I've never bothered tracking
	  it down, and i'm not even sure the above description of the
	  trigger of the problem is accurate, but that's what's seemed
	  likely to me given the menus i'd accepted right before
	  sysinst hung.  sysinst was killable (via some kind of
	  signal, I forget which 8-).


Obviously, if anybody's interested in helping with any of these tasks
i'd more than welcome the help.
	

cgd
-- 
Chris Demetriou - cgd@netbsd.org - http://www.netbsd.org/People/Pages/cgd.html
Disclaimer: Not speaking for NetBSD, just expressing my own opinion.