Brock Pytlik wrote:
client.py:
356: Is there a reason to use the string "known" here instead of a
constant?
No constant exists, and it seemed silly to create a constant for a
single case.
520: Why is the FileInUseException case being removed?
No idea; accidental deletion? I've restored it.
Comments on the change from passing in img_dir to img:
I think this is going the wrong direction. My suggestion is to instead
do the __api_alloc around line 2484. Then, if a function uses the api
object, pass it in, otherwise grab the root from the api and pass that
in (until we finish moving all the functions over to the api, then we
can stop doing that). Is there a reason not to handle things that way?
I disagree. The whole point was to create an image object, in a single
place, once.
The problem is multi-fold:
* Various routines want a quiet or a not-quiet progress tracker, and
that needs to be done at api object instantiation.
* The client has special error handling that it wants to use before any
sub-command is executed if img find_root() fails.
* There is a not-to-trivial penalty in constructing the image object
twice. In particular, when a v0 image is loaded by the new client code,
the cost of conversion of the existing data is rather expensive (45
seconds on my system). So it is vital that the image object only be
created once. This is documented in the relevant bug, but I should have
explained it in my review request.
api.py:
I realize this is just a temporary arrangement, but having a required
argument of img_path, and optional one of img, and having img override
img_path as if it didn't exist, seems wrong and broken to me. If we need
to be able to pass an image into __init__ (I'm not sure why we need to
yet, but I'm still working through things), I suggest we allow either an
image, or an image path to be passed as the first argument to init. The
init function can look at the type of what it was handed and decide what
to do appropriately.
There's a subtle problem in how we handle api compatibility exceptions.
That is, I can't make img_path=None since doing so would require
placing it after all the other arguments. Doing that would in turn
cause clients using an older version of the api to *not* die gracefully
with the incompatible api exception message.
However, the solution you've provided works nicely for the moment. That
is, I'll drop 'img=None' and just do an isinstance on img_path. I don't
even want to document this option's existence really as I plan on it
being removed. It's purely a hack for client.py at the moment.
image.py:
Is there a reason we've removed the validation option for
refresh_publishers?
Yes. It's an implementation detail I no longer want to expose, and it
seemed pointless as the transport system appears to do this as needed
anyway.
897-898: Shouldn't these be in separate try blocks? If the state was
marked as preferred, but not installed, setting this to be uninstalled
wouldn't remove preferred. Also, if you're just going to pass on the
A package can't be preferred unless it was installed, so my reasoning
was that there was not point in removing preferred unless installed existed.
KeyException, you should probably use discard instead of remove.
Ok.
I don't understand the comment on 917-918
Because the 'installed' catalog is simply a subset of the 'known'
catalog, the way to ensure that the 'installed' catalog properly
reflects that is to always remove the corresponding entry and then
re-add it with the 'known' catalog state information.
2276: Is there a reason to make this change?
More efficient as it avoids assignment to a variable that is immediately
discarded? That and pylint complained ;)
imageplan.py:
349: is there a reason to change from f, st to res?
Yes; more efficient. See above.
publisher.py:
946: why do the import here?
Avoids the overhead of importing the module until it's needed.
General question, what's a "meta_root"?
A location to store metadata :)
manifest.py:
481-482: why not just compare signatures with self.signatures?
Because that would be a self-fulfilling prophecy :)
The signatures are generated based on the current contents, so validate
would always be true.
This allows an external caller with the signature data they have for the
manifest to validate easily against what the manifest thinks its
signatures are.
server/catalog:
We could probably just remove the checks for .pag and .dir? Those
haven't existed since the 2008.05 release.
1151: what are the possible classes that could be coming in here?
It's marked as a classmethod now so it can access __parse_entry.
pull.py:
Am I right that this means that pkgrecv isn't moving to the new catalog
format (yet?)
Correct; Phase III.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
