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.
- 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.
- 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.
OK. I log a warning now.
- 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.
- 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
