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. > > 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. (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.) 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
