Thanks Sue - I'll update those prior to pushing.
- Keith
On 02/ 1/11 02:07 PM, Sue Sohn wrote:
Keith,
Before you push this, a few typos/nits:
84 the->then
91 capture->captured
92 its->it's
89-101 could use some whitespace
Sue
On 02/ 1/11 01:55 PM, Karen Tung wrote:
Hi Keith,
The updated code is *much* more readable now. I understood everything
with just 1 pass through the code. :-)
I have no further comments.
Thanks,
--Karen
On 02/ 1/11 12:59 PM, Keith Mitchell wrote:
Karen,
I've updated the webrev and posted it to:
Differential: http://cr.opensolaris.org/~kemitche/webrev.15957.2.diff/
Full: http://cr.opensolaris.org/~kemitche/webrev.15957.2/
Hopefully, the data and code structure is a bit more readable now.
Please let
me know what you think.
Thanks,
Keith
On 02/ 1/11 12:35 PM, Keith Mitchell wrote:
On 02/ 1/11 11:41 AM, Karen Tung wrote:
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.
I agree that readability is an issue. I'll look at revamping names and
structure a bit to improve this.
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?
I initially tried that, but found that this method essentially led
to code
that was twice as long and peppered with things like:
if fileno == stdout_fileno:
# 5-10 lines of code
else:
# The same 5-10 lines of code with stdout replaced by stderr
It ended up seeming very "clunky"
The other complicating factor is that, sometimes, self.stdout or
self.stderr
is None (if they aren't being piped, stored, and/or logged). Which
means the
logic has to do a number of "is not None" checks.
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'll try to flesh that out a little better. I wanted to keep the data
structures simple rather than generating many complex classes in
support of a
"simple" Popen class, but some structure will probably help clarify.
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
_______________________________________________
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
