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/[email protected],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/[email protected],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
[email protected]
[email protected]
[email protected]
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
ent...@[email protected],5.11-0.134
[email protected],5.11-0.134:20100302T023003Z
pkg://pkg.opensolaris.org/[email protected],5.11-0.134
pkg://pkg.opensolaris.org/[email protected],5.11-0.134:20100302T023003Z
-Sanjay
-Sanjay
_______________________________________________
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
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
