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
