Danek Duvall wrote:
Shawn Walker wrote:
http://cr.opensolaris.org/~swalker/pkg-cat-p3/
...
pkg.depotd.1m.txt:
...
I guess the following comments are a bit late, so take them with a grain
of salt for this wad. But you probably should be thinking about them for
the next release.
- line 251ff: I thought you guys were working on making multiple
publishers from a single depot more possible; this seems like a step in
the wrong direction.
It just needs clarification. This is really the "default" publisher.
At some future point there will likely be additional configuration
options when the configuration format is more expressive than
ConfigParser allows.
- line 258: "collection-type" instead of "collection_type", et al.
- line 268, 272: using "uri" in some places and "url" in others is
confusing. Also plural vs singular.
Yes, they're inconsistent, and yes the '_' vs. '-' is noted. I've been
deferring this work to the future repository configuration overhaul.
The primary thing I was trying to do in this wad was to document them so
that they could be used with the --property option.
The plurality is a result of compatibility with previous depot
configuration.
The URI vs. URL was my personal nitpicking about accepted terminology in
documentation per recent RFCs (notably, URL is considered obsolete).
- line 309: Should this be split into related repos and other related
URIs?
The intent was to only account for repositories at the time of the
original implementation. I'd prefer to revisit this during the
repository configuration overhaul.
modules/catalog.py:
...
- line 633: Is there any way you can think of to use "key" instead of
"cmp"?
Concatenating the stem and publisher (e.g. "%s!%s" % (stem, pub)) then
doing the sort, and then splitting them apart again as I iterate over
the sorted list?
- line 2201: why is this an exceptional condition, rather than just a
return of an empty list, as on line 2190?
Line 2190 is wrong and is a holdover from when the catalog.attrs file
was being conditionally retrieved. I've changed this.
publisher.py:
- line 1198: "if flist"
- line 1212: just put this at the top, I think.
Previously this was to avoid a circular import issue, but that appears
to no longer be a problem.
depotcontroller.py:
- line 94: don't need the last ", None", I think.
Wouldn't that raise a KeyError if the first condition returned {} ?
server/catalog.py:
- Seems like more could be ripped out of here, like the search related
stuff?
Correct. I intend to rip most of it to shreds in the near future.
Because of the amount of churn involved, I wanted to defer that. So,
I've opened a bug instead (12123).
server/repository.py:
...
- line 488: why have private methods to return private variables?
See lines 1240, 1241, 1242. These are accessor methods for the
property() declarations.
- line 770: this isn't safe; why not just copy old_cat_root into
tmp_cat_root? Or copy the contents of old_cat_root into tmp_cat_root?
Isn't safe in what manner? The temporary directory is being created
within the repository root or within the writable_root, and multiple
pkg.depotd processes can't modify the repository. Or were you talking
more about the "guaranteed, unique" aspect?
As for why? Because I was trying to avoid manually walking the
directory structure and shutil.copytree() requires that the destination
directory not already exist. Do you have any suggestions that don't
involve a manual directory walk?
- line 805,806: this isn't particularly safe, either, but I'm not sure
what else to suggest, unless you can put an in-process lock around the
use of catalog_root.
The locking picture is not yet complete.
However, all of the things that call __save_catalog() use a thread lock
(see __lock_repository()) so that should be sufficient for the moment, I
think.
My assumption was that whenever bug 2696 is implemented for repository
locking, that more would be done here. At that point, I also plan on
adding multi-threaded tests to the test suite for the repository.
In particular, this will eventually be needed since I've been told that
the ON build team would like to do parallel publication, which is
currently not supported (that is, multiple packages at once via file://).
- abandon, add, add_package, and open all follow a common pattern. You
might consider a decorator to do that for you.
I'd prefer to defer this particular bit as I'm not keen on possibly
breaking this (plus I've never written my own decorators so I need to
learn the pattern first). Can I open a bug?
server/transaction.py:
- line 177: can you pass an fmri like "pkg:/SUNWcs" to the depot? If so,
then this could result in "pkg://opensolaris.org/pkg:/SUNWcs".
I feel dumb here. If you passed 'pkg:/SUNWcs', the FMRI would pass the
"not startswith('pkg:/')" test and would replace 'pkg:/' instead of
pre-pending. Can you explain?
extra_privs:
- is this change really part of this wad?
Hmm, this got picked up in a merge somehow. I don't know why it thinks
I added this file, since that's clearly part of changeset 1393. Looking
at the Mercurial log though, it thinks I added it as well. How to fix?
search.shtml:
- val = bool(val)
Thanks for responding so quickly. The review is greatly appreciated.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
