Hi Jack, I have some more comments, sorry...
- Nit: lines 173, 632 and 704 - "Insure" should probably be "Ensure" - line 300 (get_progress_estimate()) This causes a remote file to be downloaded - since this method is called really early on in the execution cycle (possibly long before the execute() method is called) - this would mean there is a possible gap between the download of the package, and the install of it - and if there is an intermediate other SVR4 type install, then it could overwrite the temporary file as it's named now! The assumption for more get_progress_estimate() methods is that they only should to a "best effort attempt" - hence the term estimate - and downloading the file is going too far at this point I think. I would still prefer if the size used here was that available in the HTTP header - it is correct to use since that is how much really needs to be downloaded regardless of how much in the data-stream you really want to install. If the size cannot be estimated, then I think a fall-back default value is all we can use. The estimation really shouldn't need to modify the filesystem in any way. I know I raised the idea of downloading the file, but given the size of the downloads involved in getting header information that you provided, this is overkill by us I feel. The only thing that really needs to do any downloading here is the pkgadd command itself. Non-file related comments: - lines 528 to 531 This shouldn't be necessary since AI sets the "http_proxy" environment variable early on in the install process, and this should be available to the pkgadd command when called. Temporary file related comments: NOTE: Before you do any of this, you might want to look at my comments on get_progress_estimate() first. - The use of "TMPDIR + svr4pkg + PID" for the temporary filename isn't that secure, and it might be worth considering using mktemp() or similar instead. - I don't see anything to unlink() the temporary file after execution has completed - I do see it if there is an exception, but not after successful completion. Maybe should be done at end of _transfer() or execute() methods? (before you do this look at my comments below on get_progress_estimate() first) - line 184: Why are you opening as "ab+" - this would imply you wish to append to any existing file, while I feel you really would want to truncate any existing file, and as such it possibly should be "wb+"? - line 187: Maybe we should log what's happening there, like "Downloading remote package ...", and also maybe some message about the amount downloaded so far (would be good to state as "x of X" if the size was available in the HTTP header). Thanks, Darren. On 02/09/2011 09:18, 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. >> >> - 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 _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
