Shawn Walker wrote: > src/modules/client/image.py: > lines 1347-1360: A few things: > > * By retrieving the dependency catalog entry directly, this will > keep this code from working for anything but package repositories with v1 > catalogs. This may have been intentional, since if you use > src_cat.get_entry_actions() the action data would be lazy-loaded > (triggering a manifest retrieval) which would probably be undesirable.
Other than not knowing if there was a better way in general to do this, I did it this way because if you're not serving catalog/1, then you probably don't have packages in your repo marked obsolete or renamed, so I don't care too particularly much. The fact that I'm avoiding downloading manifests is awesome, though. :) > * You should discard the OBSOLETE and RENAMED states around line > 1348 (every time) since this is a merge operation. This will ensure > that the states information accurately reflects what is in the > catalog. Unlikely to create an issue, but theoretically possible. I don't understand. What's being merged? In particular, the "states" variable is being created completely anew each call, so discarding those states from "states" won't make any difference. Am I misunderstanding the code, or your comment? > src/modules/server/transaction.py: > lines 399-441: Curious, why do this check in add_content instead of > close()? Was it an to attempt to fail as soon as possible? Yes. > One concern is that if they weren't paying attention to the exit return > code of pkgsend, and they were doing one-by-one 'add' operations, this > would appear to cause the server to fail all of the bad adds, but the > close would still succeed. Should you record the transaction error in a > list, and if that list is populated, fail on close with all of the > accumulated errors? I dunno. If we do that, then you'd have to abort the transaction and start from scratch, which could suck (if you were publishing a large package and accidentally sent it an obsolete tag). I originally had XXX comments here suggesting I should just go ahead and abort the transaction then and there, but that seemed overly harsh. Still, I'd rather do that than wait until they close before telling them (again) there's a problem. On the other hand, maybe it makes sense to have an explicit "clear error" operation that would be required before the transaction could be closed, or have any other actions added to it. But I think that can wait. > src/tests/cli/t_pkg_install.py: > > lines 2837-2841: since you've combined the publication and install > tests here, you need to test file:// publication as well. I can pull this test out and put it into t_pkgsend.py, if you think that would make more sense. Is there any reason to expect that file: repos wouldn't take the same codepath, though? > Also, although it's slower, I'd be certain to check one-by-one action > publication in addition to bulk. Why? > src/client.py: > > Any thoughts about the client not showing obsolete packages at some > future point? (Not this wad.) For example, if they've never installed a > package and the last (newest) version of it has been obsoleted or > renamed, perhaps it should be elided by default? Yeah, I suppose under those conditions. It should be shown with -f, though, as I really want to avoid yet another "no really, show me everything dammit" flag to list. > src/modules/catalog.py: > > line 1640: you shouldn't need this given line 1630 now (result of my > recent putbacks) Yup, Rich saw the same thing. I noted it as weird, but sadly it didn't trigger "aha! mismerge!". Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
