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 [email protected],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
