Hi Keith,
Please see my comments inline.
On 02/ 1/11 09:17 AM, Keith Mitchell wrote:
On 01/31/11 02:04 PM, Karen Tung wrote:
Hi Keith,
As we discussed offline, your implementation of Popen will delay
printing any stdout
and stderr to the log files until the command is completed. This is
not desirable for
application that wants to see any output/error from long running command.
Users of those application might think the command is hung if they
don't see
the expected output from the commands in the log files.
It would be best to display any output/error from commands as soon as
it is "sensible".
Thanks for pointing out that use case! I've spun up a modified webrev.
The logic for the logging case is now much different - the general
flow copied from the existing exec_cmds_output_to_log function.
The updated code looks fine from the correctness standpoint.
However, I found the way it is currently written not very easy to read
and understand.
I read it a few times before I am able to figure out the different,
but yet very similarly named variables.
IMO, since there's only 2 file descriptors we need be concerned about,
is it really
necessary to have a buffers array? Perhaps the code might be more
readable to name
each file descriptor explicitly? Furthermore, while I understand Python
does not require variables
to be declared before they are used, I found the different variable used
in keeping track
of information that needs to be saved between __log() and __do_read()
function confusing.
If you decide to use the array of buffers to keep track of stuff like
you do now, it will be
useful to put it in some comment on the different "fields" that is used,
and what's their purpose.
I see that you removed the reference to install_extra in
pkg/transforms/synthetics.
Are you also going to also remove the reference to repo.extra in
pkg/README.pkg so all
references are gone, even though it is harmless.
Per the Note at the top of README.pkg, that file was only ever a
reference and was never modified from its original source, the ON
gate. It has many references to things that don't apply to
slim_source. As such, I don't feel that maintenance of that file is
worthwhile at this time.
I didn't see the top of the README.pkg file. I just do a grep. :-)
Thanks for the explanation. It's
OK to leave the info where it is then.
Thanks,
--Karen
Thanks,
Keith
Thanks,
--Karen
On 01/31/11 12:20, Keith Mitchell wrote:
Hi all,
I'd like a code review for my fixes for:
15957 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15957> Implement
solaris_install.Popen
7014402
<http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7014402> SUNW*
packages showing up in install-redistributable
webrev:
http://cr.opensolaris.org/~kemitche/webrev.15957/
Notes:
* solaris_install.Popen is a drop-in replacement for
subprocess.Popen - as verification, the test code actually runs the
full subprocess.Popen test suite against solaris_install.Popen.
* "exec_cmd" and its family have not yet been removed. I will file
bugs against the various installers to replace usage of such
functions with solaris_install.Popen invocations; this allows for
new consumers to use solaris_install.Popen now, rather than waiting
an indefinite amount of time for me (or someone) to pull out those
functions and widely re-test a large portion of the slim_source
codebase.
* I initially considered doing a global find/replace of
subprocess.Popen with solaris_install.Popen. However, since
solaris_install.Popen is in the solaris_install module, in many
cases that results in a new Python import of a module not in the
Python standard library. This has potential implications on
packaging dependencies; as such, there are risk and time factors
involved in such a find/replace (in the form of at least sanity
checking most installers). As there will be bugs to remove
"exec_cmd" functions anyway, it seems appropriate to have those bugs
also replace subprocess.Popen with solaris_install.Popen. (The
current approach also is probably easier for the various projects to
merge with)
Thanks,
Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
