Hi Jack,
As requested by you, I am not going to review the updated
webrev until you post another version later. But I do want
to respond to a few items below.
On 09/ 2/11 01:18 AM, Jack Schwartz wrote:
Hi Karen and Darren.
Webrevs have been updated with both of your suggestions.
There were lots of changes...
Round 2 webrev vs slim_source:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_2
Round 2 vs Round 1
https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_2_1
Karen,
Thanks for your review.
More below...
On 09/ 1/11 03:04 PM, Karen Tung wrote:
Hi Jack,
Here are my comments.
General:
------------
The SVR4 admin file is just a few lines of key value pairs.
It's not something the user can customize, at this time, at least.
Is it really necessary to deliver a file? Why not put all
these key value pairs into a static dictionary in the svr4.py
file, and create the file at execution time.
This have the advantage of having everything the svr4.py module
uses in 1 single place. It also have the advantage
simplifying all the code in lines 473-496 of svr4.py, since
you can just add or reset of the value of the "proxy" key,
before writing out the file.
I created a file so the user could replace it if desired. However,
thinking more, if the user tinkers with it and causes an install or
build to fail, it's just another thing we have to support, and having
it doesn't seem to offer enough to outweigh this liability.
What would the admin file help with? There's the scenario of a
package with a conflicting file. Currently we quit on conflicts. The
admin file would provide a way around this. For example, one using AI
could change the admin file to complete their install. However if it
is not changed back, the admin file could lead to unexpected results
since it could end up being used by DC or another AI install.
The thing the admin file was to help with is easily worked around, by
the user just doing a manual pkgadd with custom admin file, after the
AI install is competed, or by doing a custom script with a manual
pkgadd in DC.
I also like the idea of using a dictionary and keeping everything
self-contained.
Seems like this change is a good idea... I'll implement it.
If you are really going to deliver the svr4 admin file, I believe
you also need to modify the package manifest to add it there.
N/A
svr4.py:
-------------
- line 41-42: are Origin and Publisher used here? Sounds like
IPS related things that's not needed here.
These things are what the module originally used. The origin is the
datastream file or directory the packages are coming from. Origin is
a child of Publisher, in the general <software> genre.
- Technically, the implementation of the get_pkgurl_status() is
correct, but I found the way the code is currently written
hard to understand. I am especially confused by lines 125-127.
For me, I would add the check to see whether the given url is
a file or a directory immediately after 115. If it is a file,
open it. That way, you can eliminate the current lines 125-127,
and just check for "if fd is not None".
I rewrote this to download remote URLs once to save on network
bandwidth, in response to Darren's comments. The replacement method
is called open_pkg_src().
- lines 157-159: Why catch the ValueError and raise it again,
just with a message? If so, why you didn't do it for the open in
line 153? My understanding with exception handling
is that unless you are going to do something useful with the
exception, just let it bubble up. If you are trying to
provide information about the value of the URL, I think it will
be easier to do it with logging debug statement.
1) Line 153 won't fail due to 152. So there's a difference between
there and 157.
2) I have changed most places which except one error and raise
another, to call self.logger.error("custom message") followed by a
raise of the original error.
- line 171: Should we also check for line != None before we get into
the while loop? I guess if line is None, the line.split call in
line 172 will fail with a trace, do we want things to fail
like that?
readline will only return None if there is some internal python
problem. It will return an empty string on EOF.
- line 172: if line doesn't have 3 parts, this won't work, and
the catching of ValueError you have in 176
doesn't include this line.
>>> a="1"
>>> a.split(" ", 2)
['1']
>>> (b, c, d) = a.split(" ", 2)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: need more than 1 value to unpack
Thanks. Moved the split into the try below it.
- line 176-178: again, I don't feel it is necessary to
catch and re-raise the exception.
Changed.
- lines 238-256: you are trying to get the size of a directory
here, right? If so, what about using the dir_size() function
from osol_install.install_utils?
Thanks. Didn't know about it. Changed.
- line 297-312: why do we need a lock? The engine only
runs 1 checkpoint at a time.
There are two entry points into a transfer checkpoint which end up
parsing the same manifest data and building the package list:
get_progress_estimate() and execute(). I don't want to put the
restriction on only one being called at a time, in case the engine
becomes multithreaded in the future. The lock and the
self.input_parsed flag insures that packages are accounted for only
only once.
I feel that is putting unnecessary complexity in the code. When an
application calls
execute_checkpoints(), the engine will first call
get_progress_estimate() on all the check points.
Then, it will call execute() on all the checkpoints. Even when we
extend the engine in the
future to allow executing multiple checkpoints at the same time, we
won't be calling
get_progress_estimate() and execute() on the same INSTANCE of a
checkpoint at the same
time. We will just be calling execute() on different checkpoint
instances in parallel.
- lines 381-400: all of this code can be replaced by
calling run(cmd), where the run() function is defined in
solaris_install. It will do much better logging and checking
of return code...etc..
Two reasons:
1) pkgadd can spew lots of output, and could fill up the buffer that
Popen.check_call() uses. (run() uses Popen.check_call().)
2) Existing code allows to check for cancellation events.
Popen.check_call() does not allow for this.
OK, in that case, you should add code to check for the return code of
the command then.
- lines 472: DC allows a user to specify a http proxy too.
I think we should handle that as well.
I don't understand this request. I already get the http_proxy from
the manifest.
Just like AI allows a user to set http_proxy, DC allows a user to do
that too
in their manifest. DC parses it like follows:
http://src.opensolaris.org/source/xref/caiman/slim_source/usr/src/cmd/distro_const/__init__.py#447
It gets it from a different DOC node than AI.
After getting the value, DC sets the http_proxy and HTTP_PROXY env
variables.
According to Darren, having the env variables set is enough, and you don't
need to do anything to the admin files. If that's true, then, you don't
need to do anything for DC since DC already sets the env variables. If
that's
not true, I think you need to add code to parse the DC's "Distro" node to
get the value.
Thanks,
--Karen
- line 525-528: is "pub" and "origin" used?
Yes. Pub on 527 and origin on 529 (of original webrev)
- line 589: I don't really see the point of introducing
a "custom" action. From what I can see in other part of the code,
we are just doing a pkgadd anyway with the special argument.
So, why not just add the special argument and leave the action
be "install"? You can put a debug/info logging statement about
using the special args.
OK. I now treat this case as a regular "install" except that I log an
info message.
- line 606: missing close ')'
Thanks.
Thanks,
Jack
Thanks,
--Karen
On 09/01/11 01:52, Jack Schwartz wrote:
Hi everyone.
Here is a code review to fix:
7084807 <http://monaco.us.oracle.com/detail.jsf?cr=7084807> SVR4
package installs using the new autoinstaller don't work
Webrev:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_1
Bug report:
http://monaco.us.oracle.com/detail.jsf?cr=7084807
Please review and bless by Friday 9/2 10 AM PST.
I have done a lot of standalone testing with a special setup.
Testing is still in progress and is looking good. I want to get the
webrev out before I complete testing so I can get this into B174,
pushing Friday COB. I don't anticipate any major changes at this point.
Testing has included remote and local datastream files with 1 and >1
packages requested, local directories which are parents to groups of
package trees.
Testing still to be done:
- A full AI install. Up until now I have bypassed IPS package
installs to save time. I'll do this first thing in the morning.
- Unit tests still need a little more work.
Note: I introduce an SVR4 admin file to AI client /usr/share/install
directory with this push. It is needed for non-interactive SVR4
installs.
Thanks,
Jack
_______________________________________________
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
