Danek, Thanks for reviewing this. I've been trying to prod Stephen into reviewing it too, since he may have a bit more context. Thus far, he seems to be occupied with other tasks.
> > http://cr.opensolaris.org/~johansen/pkg-unknown/index.html > > This seems overengineered to me, given our current requirements, which are > pretty minimal. (I have the feeling that you weren't quite sure what to do > here.) I was thinking more along the lines of putting checks at 552 and > 606 to make sure that cv was "in 'CV'", and perhaps ensure that pkg == > "pkg". Uh, yes, kindof, but not really? The original version of this code simply ignored any catalog entry that we didn't understand. When we downloaded one, we ignored it, and omitted it from the catalog. If one happened to sneak into a catalog, we just ignored it too. However, if we're downloading incremental updates to the catalog, or any catalog with entries we don't understand, the client doesn't have enough information to convert the entry from the server-side format to the client-side format. We're saving the prefixes that we've seen, or can convert, so that once we run a client with code that can process a server-to-client format conversion, we go back and convert the stuff we didn't know how to handle. > I'm not sure I understand the point behind the transformations you > allude to; perhaps an example would help? But if we're not doing anything > like that now, why complicate the code with all this stuff that basically > won't get run? At the moment, these transformations are how we convert from server-side catalog format to client-side catalog format. This code will get run as soon as we start introducing new catalog entry types. The old clients have to cope, and the new clients shouldn't get screwed up by unconverted catalog entries. I'll be adding new types for the manifestlist/depend stuff, as well as the package rename stuff. I'm also pretty sure that Stephen's incorporations code will need a new type too. It will get used. > Still, there are some comments on the implementation choices that I believe > are worth making. Thanks for taking the time to do so. > catalog.py: > > - line 37 - no need for this; "set" is a built-in type as of 2.4 Removed, Thanks. > - line 742 - just keep this as a string -- "CSV" -- as that's already > immutable, and trivially becomes a set with "set(known_prefixes)". Changed. > - line 228 - "if new:" ought to be sufficient here. Changed. > - line 229ff - other than the calls to added_prefix(), all this is doing > is setting the "prefix" attribute to "known_prefixes". It's adding any new prefixes in known_prefixes to the "prefix" attribute and writing it back to the attributes file, yes. > - wouldn't we want to check if any of the prefixes in the catalog aren't > recognized (i.e., "if pfx_set - known_prefixes:")? This is a good question. If any prefixes in pfx_set aren't in known_prefixes, we'll just skip them as most of the code only checks the known_prefixes list. If something is in pfx_set that isn't in known_prefixes, it's likely that someone ran an old version of the client, used a new version, and then used the old version again. I'm assuming that in this situation we want the older client to be able to get work done. > - line 546ff - so I would have just done this first test after the > split() on line 551 -- "if cv not in known_prefixes:". But the check > should actually be more specific here, because this bit of the code can > only handle "C" and "V" types. In fact, adding just that check was > pretty much the intent of this bug. I don't think I understand. In what way should this check be more specific? > - Similar comment for the hunk at line 600, though here you don't do a > check inside the try clause, but you do in the except, which is > confusing. I took a look at this code, and I'm confused by this comment. The check against known_prefixes happens outside of the try both here and at line 546. The check if pkg == "pkg" is in the try in both cases. > updatelog.py: > > - it occurs to me that we don't have any documentation of what the format > of an update log looks like, the way that Catalog's docstring does. I'll add this, but it's basically + or -, a timestamp, and then the catalog entry in serverside-format. > - It's probably worth putting a XXX comment in here to at least log a > warning when we retrieve an update log that contains an entry we don't > understand. I'm not sure it's a great idea to emit a warning every time we read a catalog line we don't understand. Do we really want clients doing that when we've introduced a bunch of new catalog types? -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
