Brock Pytlik wrote:
Shawn Walker wrote:
Greetings,
The following webrev contains changes and fixes for the following
issues and RFEs:
5872 List APIs required
12929 image.py:get_publisher_ranks can traceback after older client
configuration changes
12946 pkg list should provide way to list only newest versions of
all packages
webrev:
http://cr.opensolaris.org/~swalker/pkg-list/
[snip]
High level comment, I'm not sold on the idea of "
Packages that do not apply to the current
image (such as those for a different architecture or that are
not applicable to the current zone type) will never be listed.
"
I'd kinda like an option to say, no, I really mean ALL packages. Mostly,
I'm thinking about a case where a user has two images in two
different situations (one on sparc, one on x86 or one that's a global
zone and one that's a non-global zone) and one is just really
hosed and they're trying to use the other to gather information about
what packages are available that might be missing.
So you're ok with architecture filtering, but not zone variant filtering
then by default?
I'm of the belief that most users are not interested in packages that
are not for their image variant by default.
However, I did make it so that its trivial to include packages not for
the current variant with the API.
I could easily add a command-line option that causes the cli to include
variant packages. Would this suffice, and if so, what should that
option be?
client.py:
I'm not sure the division between client and api here is quite right. I
think more of the logic here should be moved into the api.
For example, the items on lines 294-308 I would think should be within
the logic for get_pkg_list? Wouldn't the gui want to refresh when the
cli would?
I tend to disagree here. The API provides a refresh function, and
adding a refresh function to get_pkg_list would then require a transport
lock. Since clients can simply call refresh() themselves if they so
desire, I saw no need to combine two different API functions.
And actually, no, the GUI actually wouldn't. I suspect different
clients have vastly different refresh behaviour. For example, the GUI
refreshes on startup and various other places after operations complete
and so on. It also triggers refreshes through the updatemanager.
I know it's a nit, but could you rename pkg_list to pkg_selection or
something like that?
324: I'd prefer we make this function (get_preferred_publisher) part of
the api instead of leaving this stray reference directly to the image
object which I thought we wanted to remove from clients of the api.
Eventually, I'd like us to pass a api_inst instead of an image object to
functions like list_inventory or search or list_contents so that we
don't have this dual mode of image and api objects.
Actually, it is already part of the API and I had simply forgotten to
convert the rest of the calls in list_inventory to API calls. I've done
that now. All of the history and pref publisher stuff is already in the
API.
generic.py:
117-120: Why the duplication of code here?
Speed. Assignment here instead of a function call is measurably faster.
Since this is a hot path for action construction, it was desirable.
I've added a comment to that effect.
catalog.py:
1475-1482: Just curious why the change to not filter out the facet or
variant actions?
Sort of pointless performance wise. I'm never expecting set
name=variant.* or set name=facet.* tags to have variant or facet tags
themselves :) This also mirrors the behaviour of the merge program that
creates the variant tags (it doesn't tag variant tags with variants, it
creates them based on the unique set of variants used in the package).
version.py:
468-484: why not return False to break out early if any of these tests
fail?
For example, instead of lines 465-469, why not:
if not ((other.release and self.release and
other.release.is_subsequence(self.release)) or not other.release or
other.release == '*'):
return False
I guess with all the other performance changes, I'm surprised this one
wasn't made.
Sort of got tunnel vision here; it was this way before so I left it that
way. I've changed it though, although I've seen no appreciable
difference in performance (oddly enough).
508: extra blank line?
532-534: Could you rewrite this as "Takes an assumed valid... and splits
it into its components ..."? I read this sentence 3 times before I read
it with the correct meaning.
Changed.
t_pkg_list:
Given that we know have t_api_list, are there tests her that could be
removed, or moved into t_api_list which is likely faster? (I'm fine with
this just being filed as a bug for future consideration.)
Some of these possibly, although this test suite completes fairly fast,
and we have to test that the cli is using the API correctly so gives the
expected results. I'll file a bug for investigation soon.
This is something I've frequently wrestled with in writing unit tests
(avoiding unnecessary test duplication).
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
