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.
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.
svr4.py:
-------------
- line 41-42: are Origin and Publisher used here? Sounds like
IPS related things that's not needed here.
- 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".
- 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.
- 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?
- 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
- line 176-178: again, I don't feel it is necessary to
catch and re-raise the exception.
- 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?
- line 297-312: why do we need a lock? The engine only
runs 1 checkpoint at a time.
- 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..
- line 426-427: _transfer_list is an internal attribute for
the AbstractSVR4 class. It's not something the user specified.
I don't think it needs to be checked here. If it's empty, then, no
installation is done. Perhaps we can log a warning if that's
the case.
- lines 472: DC allows a user to specify a http proxy too.
I think we should handle that as well.
- line 525-528: is "pub" and "origin" used?
- 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.
- line 606: missing close ')'
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
