Shawn Walker wrote:
> Brock Pytlik wrote:
>> Shawn Walker wrote:
>>> Brock Pytlik wrote:
>>>> Here's the link:
>>>> http://cr.opensolaris.org/~bpytlik/ips-api-v1/
>>>>
>>>> I haven't done as much cleanup as I usually try to do, but since
>>>> I'm ooto tomorrow, I thought it would be better to get this out
>>>> now, rather than next Monday.
>>>>
>>>> Other things that I know need to be improved, but that I think can
>>>> wait for another round of revisions:
>>>> API Exception organization
>>>> More complete functionality
>>>> Automated testing of cancellation (I hope to work with the GUI team
>>>> to test this by hand in the interim)
>>>> Versioning of the Progress tracker (perhaps)
>>>> More Documentation
>>>> Figure out the future approach to testing we'll take
>>>
>
>>> lines 1668, 1672, 1674, 1676, 1684: You should pass img.imgdir
>>> here instead of mydir as the image object could have a different
>>> value for img.imgdir (see lines 190-191 of modules/client/image.py).
>> I'm pretty sure I want to be passing in mydir since, eventually,
>> client won't know about Image's at all. Can you help me understand
>> how the existing code could cause problems, and in what ways the new
>> code would blow up? What I might do is pass in a API object, instead
>> of the dir.
>
> Because the find_root changes the effective value of mydir. If you
> read through find root, you'll notice it uses realpath(), etc. to
> ensure relative paths get transformed and so forth. In short, I don't
> think you should be using mydir as that is possibly a relative path or
> isn't necessarily the *real* image directory. We could change the
> logic for an image directory later...
>
> You should probably use an api object here or you need to pass
> img.imgdir. What you have currently, in theory, could regress some of
> the fixes that went into related to relative paths and so on.
>
>
Right, except that the __init__ method of PkgClient does the call to
img.find_root, so that should prevent all regression I'd think.
Part of the idea behind this structure is to hide all of that kind of
complexity from the client :-)
>>> line 1713: it seems like we should call api.cancel() here in the
>>> event of a KeyboardInterrupt or a KeyboardInterrupt should
>>> automatically trigger a cancel in the api itself somehow.
>> I don't think I agree, at least not right now. For starters,
>> api.cancel() is a blocking operation, so it would delay the program
>> from actually exiting. Also, at least until there's on-disk state
>> it'd be necessary to clean up, I'd tend to pass. On the other hand,
>> if you're saying we should change our user interactions so that, for
>> example, hitting ctrl-c while the package is actually being installed
>> should simply ignore the users wishes, then I'd be more receptive to
>> this change.
>
> I'm not sure what the final implementation should look like, I'm just
> concerned that a user triggering a KeyboardInterrupt doesn't cause an
> api.cancel(), and therefore, could prevent a proper cleanup of the
> operation. I don't think we want to ignore the user's wishes as
> that's likely just to make them use "kill -9".
Right. Right now, there's no state to clean up, since nothing is written
to disk. Or, to put it more simply, right now ctrl-c won't do anything
different than it does today. I agree, we probably need to decide what
the proper behavior is in different situations, and that may involve
calling api.cancel. But, I don't think that putting it in the main_func
of client.py is the right place to put it. Also, I think that's a major
project itself, and one I'd rather not shoe-horn in to all this change.
>
>>> line 591: UNINSTALLED doesn't seem right here. I'd use the states
>>> presented in doc/pkg-states.txt, which in this case would appear to
>>> be DELETED if it was previously installed or IN_CATALOG if it's
>>> never been installed.
>> For now, I'm going to stick with Uninstalled since that's the info
>> that the client needs. Also, I'm not aware of a way to retrieve
>> information to determine whether a package is DELETED or IN_CATALOG.
>> If we have that info somewhere, I'll change the states as you suggest.
>
> We don't have that information right now, but please don't use
> UNINSTALLED. It's, in my view, inaccurate and misleading -- consider
> NOT_INSTALLED as a viable alternative.
Ok, I like NOT_INSTALLED.
>
>>> line 1: anything that inherits from exception should have it's own
>>> __init__ or have its own __init__ and call exception.__init__
>> Init isn't inherited from Exception? If so, why should I create an
>> init method which only does Exception.__init__(self). Doesn't the way
>> OO works make that happen anyway? I wasn't aware __init__ had
>> different semantics for inheritance than other methods.
>
> Sorry, ignore this then. This was my mis-reading of the related
> Python docs [1].
>
> Cheers,
Thanks
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
