Hi Jack,
I really appreciate your efforts on this and the quick turn around your
making...
I'm much happier with things now - Karen and Drew have pointed out several small
issues, I don't have much else to add to them, other than a tiny nit:
320 + if progress_estimate == 0:
321 + progress_estimate = 1
Although it's possibly unlikely, I think it makes sense for this to read as
"progress_estimate <= 0" to be sure we catch all possibilities...
Thanks,
Darren.
On 04/09/2011 14:49, Drew Fisher wrote:
> Jack,
>
> I missed all of the discussion on Friday, but I wanted to throw my hat
> in the ring.
>
> Note: I've only looked at the most recent version 4 vs. slim source webrev.
>
> svr4.py
> ----------
> nit: Almost all of the new logger messages are going to .info() For
> errors the user needs to be aware of, I'm fine with it, but the more we
> can move to .debug(), the better. It drastically improves readability
> of what goes to the screen.
>
>
> 66: change to: ROOT = os.environ.get("ROOT", "")
>
> so you don't have to do lines 67-68
>
> 85: Why not change [A-Za-z]* to \w*
>
> 107-109: If this exists solely for testing, it needs to move to the
> test file
>
> 251, 287: use list()
>
> 300: can you add a space after both ":" characters, for legibility.
>
> 320: change to: if not progress_estimate:
>
> 419, 425: align with previous line
>
> 587, 590, 691, 694: change both of these to a simple 4 space indent
>
> 603-608: I don't understand why this block of code is here and
> commented out. If it's only for testing, move it to the test file. If
> it needs to be supported by the DTD files, we should file a bug and get
> it fixed.
>
> -Drew
>
>
>
>
> On 9/3/11 4:12 PM, Jack Schwartz wrote:
>> Hi Karen.
>>
>> Thanks again.
>>
>> Next iteration vs slim_source:
>> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_4
>>
>> vs V3:
>>
>> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_4_3
>>
>> Replies below. Please bless.
>>
>> On 09/ 2/11 06:18 PM, Karen Tung wrote:
>>> Hi Jack,
>>>
>>> I reviewed
>>> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_3
>>> and here are my comments:
>>>
>>> ----------
>>>
>>> - General comment about svr4.py: I see that you are using all the class
>>> constants (ie: all those that are defined before __init__()) as
>>> self.XXXX.
>>> I guess it works, but I don't think that's how it is typically used.
>>> I looked at all the other
>>> python code we have, and all the class constants are used as
>>> classname.constant_name, for example, AbstractSVR4.ADMIN_FILE
>> Some modules use self, some don't. For example, in the same dir, I
>> see ips.py uses self. cpio.py uses a mixture, media_transfer users
>> the class name. I'm happy to change this per your request.
>>>
>>> - Another general comment: I still don't think you need a lock for
>>> the parse_input. I understand your concern about multiple threads
>>> calling parse_input at the same time. Since we have full control of
>>> the CUD
>>> architecture, we know that get_progress_estimate() and execute() won't
>>> get called at the same time.
>> Since it's apparent that you feel very strongly about this, I'll
>> remove it and add a comment to the code that it is not thread-safe.
>>>
>>> - 89-91: since you are defining these as constants, it would make all
>>> the debugging statement much more readable if you define them as
>>> strings,
>>> so, I don't need to figure out what's "1". Also, I think all these
>>> should be capitalized.
>> OK
>>>
>>> - line 364: should we check whether the admin file already exists?
>>> If so, should we just use it? If not, should we log a warning about
>>> the fact that we are overwriting the file. Also, I think the
>>> admin file should be removed when the checkpoint with it.
>> Relocating the file to _execute and deleting it when through with it.
>>>
>>> - line 479: I don't think you should return here. Other values
>>> should still be checked.
>> The rest of this method is a for loop on the empty list, functionality
>> is the same with or without the return. I'll remove the return to
>> make it easier in case someone modifies the routine adding stuff to
>> the bottom of it though.
>>>
>>> - line 545-548: since the admin file is not used until execute()
>>> time, should
>>> we not generate it until we really need to use it?
>>
>> Relocating generation to execute() and deleting it when through with it.
>>
>> Thanks,
>> Jack
>>>
>>> ----------
>>>
>>> Thanks,
>>>
>>> --Karen
>>>
>>>
>>>
>>> On 09/ 2/11 04:34 PM, Jack Schwartz wrote:
>>>> Hi Darren.
>>>>
>>>> Hopefully this webrev addresses all of your concerns.
>>>>
>>>> Webrev vs slim_source:
>>>> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_3
>>>>
>>>> Webrev vs V2
>>>> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_3_2
>>>>
>>>> This version does no downloads of remote files but uses http
>>>> protocol to get remote datastream sizes. Only pkgadd does downloading.
>>>>
>>>> BTW, I can now also say that I've done a full AI install that
>>>> includes an SVR4 package. It works in that the data shows up on
>>>> disk, but I cannot login due to other issues.
>>>>
>>>> Responses below.
>>>>
>>>> On 09/ 2/11 04:11 AM, Darren Kenny wrote:
>>>>> 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.
>>>> That's now how it is.
>>>>
>>>> Sizes are a little inconsistent but shouldn't be a problem. Remote
>>>> datastreams use http protocol to get size of the whole image. Local
>>>> datastreams do what was done earlier by looking at the datastream
>>>> header. Local directory trees use dir_size() to get the size.
>>>>
>>>>
>>>>>
>>>>> 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.
>>>> Removed. I verified that pkgadd uses http_proxy environment
>>>> variable as well.
>>>>>
>>>>> 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.
>>>> N/A
>>>>>
>>>>> - 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)
>>>> N/A
>>>>>
>>>>> - 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+"?
>>>> N/A
>>>>>
>>>>> - 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).
>>>> N/A
>>>>
>>>> Thanks,
>>>> Jack
>>>>>
>>>>> 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
> _______________________________________________
> 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
