Hi Sanjay, This is just a small style suggestion: In Python when you have a comment block that is longer than one line, the style guidelines recommend to use ''' comment '''.
# comment is kept for to the side of the code ''' is usually used for blocks of comment that explain code below ''' My two cents :) Luis Ethan Quach wrote: > Hi Sanjay, > > Thanks for making the changes. They look good. > > But you never answered my first comment: > > >> 1231 - Is this going to work with hierarchical package names? e.g. > >> > >> > pkg://opensolaris.org/system/library at 0.5.11,5.11-0.135:20100312T101036Z > >> > > So I'm assuming then that 1231 would yield "system/library" here then ? > > > thanks, > -ethan > > > On 03/25/10 23:08, sanjay nadkarni (Laptop) wrote: >> An updated version of the webrev is available at: >> http://cr.opensolaris.org/~nadkarni/bugfix_15132-1/ >> >> On 03/25/10 01:06 AM, sanjay nadkarni (Laptop) wrote: >>> On 03/24/10 06:33 PM, Ethan Quach wrote: >>>> >>>> >>>> On 03/24/10 16:38, sanjay nadkarni (Laptop) wrote: >>>>> On 03/24/10 05:19 PM, Ethan Quach wrote: >>>>>> >>>>>> >>>>>> On 03/24/10 15:19, sanjay nadkarni (Laptop) wrote: >>>>>>> On 03/24/10 02:31 PM, Ethan Quach wrote: >>>>>>>> Hey Sanjay, >>>>>>>> >>>>>>>> [path to updated CR lacks your ~userid btw] >>>>>>>> >>>>>>>> >>>>>>>> Couple of questions: >>>>>>>> >>>>>>>> 1231 - Is this going to work with hierarchical package names? e.g. >>>>>>>> >>>>>>>> pkg://opensolaris.org/system/library at >>>>>>>> 0.5.11,5.11-0.135:20100312T101036Z >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> 1217-1235 - It seems to me that with this code, the last instance >>>>>>>> of a >>>>>>>> package (for the edge and perhaps error case that a package >>>>>>>> might be >>>>>>>> specified more than once for whatever reason) would just take the >>>>>>>> last >>>>>>>> instance of the package and throw way earlier ones? >>>>>>> Taken by itself that is true. However, even before this specific >>>>>>> code is >>>>>>> called, the entire list of pkgs are validated and such errors >>>>>>> will be >>>>>>> caught hence it is reasonable to assume that we have a valid list >>>>>>> when >>>>>>> this function is called. >>>>>>> >>>>>>> Does that help ? >>>>>> >>>>>> Yes, it does. Could we add a comment stating that that is what this >>>>>> code does then? Its just that its not obvious looking at the >>>>>> code, and >>>>>> having that would be helpful down the line in case we need to debug >>>>>> something in this realm. >>>>>> >>>>>> (and while I'm not aware of where we actually do sanitize the >>>>>> pkglist for >>>>>> the AI case before getting here, I agree that this code wouldn't be >>>>>> the >>>>>> place do that level of validation anyway.) >>>>>> >>>>>> >>>>>> And so given the above, one thing to point out is that a pkglist >>>>>> with >>>>>> repeated pkgs would have previously been processed by transfer by >>>>>> successively calling pkg install on each of them. For example, if >>>>>> in the >>>>>> list we had >>>>>> >>>>>> SUNWfoo at 5.2 >>>>>> SUNWfoo at 6.5 >>>>>> SUNWfoo at 4.3 >>>>>> >>>>>> transfer would have simply tried to install all of them (I'm not >>>>>> even >>>>>> sure >>>>>> what the behavior of pkg would be when the latter two are called to >>>>>> get >>>>>> installed) >>>>>> >>>>>> But with our changes here, we're kindof silently ignoring the first >>>>>> two, >>>>>> and just installing the last. >>>>>> >>>>>> If this is a supported list, I'm wondering if we shouldn't just >>>>>> pass all >>>>>> three of these to 'pkg' in the batch, and have it decide what to do. >>>>>> Seems that could be achievable if the values in the pkgdict were a >>>>>> list per each key, instead of a single value. >>>>> I am not sure if that is valid or not ? Having a list for pkgs is >>>>> not a >>>>> bad idea if that is the case. >>>> >>>> I just asked an ips developer this question, and specifying three >>>> different >>>> version instances of a package is not valid. But per the reason below >>>> [1] >>>> I would still like to see this suggested change in the code. >>>> >>>>> >>>>>> >>>>>> If such a pkglist is not supported, then we need something >>>>>> earlier in >>>>>> AI to validate against it. If this is the case, I wouldn't expect >>>>>> that to >>>>>> be addressed with your changes, but a separate bug should be filed. >>>>>> >>>>> This entire code is a workaround for the ordering issue in IPS and >>>>> hopefully will be deleted soon (see the comment!) >>>> >>>> Understood, but the code that handles the ordering issue is >>>> essentially >>>> only lines 1249-1255 isn't it? (well plus the bit of for loop code at >>>> 1260 >>>> that handles the fact that we have two batches instead of one) >>>> >>>> But the rest of the code handles the generic change to batch the >>>> pkglist, >>>> which I'd assume will remain post that IPS fix. >>> Actually none of this code will remain and by that I mean, the >>> classes, the methods defined in the class etc. IPS interactions will >>> use the API defined by pkg.client.api and if the ordering issues are >>> addressed, then the complete list of packages will be sent as a single >>> list. >>> >>>> >>>>> >>>>> Please let me know if you want me to address this. >>>> >>>> [1] Given that with the current changes, we'd end up with a successful >>>> install (by silently ignoring the first two instance of the repeated >>>> package) >>>> instead of at least failing, I think we need to address that. Passing >>>> all >>>> three to pkg will cause a failure, which I think is the right thing >>>> to cause. >>>> >>> okay. >>> >>>> >>>> (Separately, I do think we need to file an additional bug against the >>>> installer in general to be able to inform the user of a bad pkglist >>>> upfront, >>>> before installation begins. But this is an existing issue.) >>> >>> Sure. >>> >>> -Sanjay >>> >>>> >>>> >>>> thanks, >>>> -ethan >>>> >>>>> >>>>> -Sanjay >>>>> >>>>> >>>>> >>>>> >>>>>> thanks, >>>>>> -ethan >>>>>> >>>>>> >>>>>>> >>>>>>> -Sanjay >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> thanks, >>>>>>>> -ethan >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 03/24/10 11:37, sanjay nadkarni (Laptop) wrote: >>>>>>>>> The code review with the change is now available at: >>>>>>>>> http://cr.opensolaris.org/bugfix_15132 >>>>>>>>> >>>>>>>>> -Sanjay >>>>>>>>> >>>>>>>>> >>>>>>>>> On 03/24/10 09:52 AM, sanjay nadkarni wrote: >>>>>>>>>> On 03/24/10 07:47 AM, Dave Miner wrote: >>>>>>>>>>> >>>>>>>>>>>>> 1249-52: I'd think you could turn this into simply >>>>>>>>>>>>> >>>>>>>>>>>>> pkgs = " ".join(order.values()) >>>>>>>>>>>>> >>>>>>>>>>>> That will not work, since order is a list of keys not a >>>>>>>>>>>> dictionary. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yeah, missed that. Fine then. >>>>>>>>>>> >>>>>>>>>>> Dave >>>>>>>>>> I have a fix that addresses the primary concern that you >>>>>>>>>> expressed >>>>>>>>>> i.e. will keys work if the pkg is expressed as a complete FMRI >>>>>>>>>> and I >>>>>>>>>> will be posting the webrev soon. >>>>>>>>>> >>>>>>>>>> The supported formats can be any of the following. I am using >>>>>>>>>> entire >>>>>>>>>> as an example here. >>>>>>>>>> entire >>>>>>>>>> entire at ent...@0.5.11,5.11-0.134 >>>>>>>>>> entire at 0.5.11,5.11-0.134:20100302T023003Z >>>>>>>>>> pkg://pkg.opensolaris.org/entire at 0.5.11,5.11-0.134 >>>>>>>>>> pkg://pkg.opensolaris.org/entire at >>>>>>>>>> 0.5.11,5.11-0.134:20100302T023003Z >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -Sanjay >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -Sanjay >>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> caiman-discuss mailing list >>>>>>>>>> caiman-discuss at opensolaris.org >>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> caiman-discuss mailing list >>>>>>>>> caiman-discuss at opensolaris.org >>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>> _______________________________________________ >>>>>>>> caiman-discuss mailing list >>>>>>>> caiman-discuss at opensolaris.org >>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>> >>>>>>> _______________________________________________ >>>>>>> caiman-discuss mailing list >>>>>>> caiman-discuss at opensolaris.org >>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>> _______________________________________________ >>>>>> caiman-discuss mailing list >>>>>> caiman-discuss at opensolaris.org >>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
