On Fri, Oct 4, 2013 at 11:22 AM, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
There is A LOT going on in this patch. Please try to break your patches up into smaller changes. At least mtree and hook support definitely should have been individual patches.
Hi, Andrew, Thanks for the code review! Sorry about the size of this patch. It's really due to the fact that these things grew organically. If I had been known before I started exactly what I needed from the mtree and hook support in order to meet my goals, well, then I would have won several lotteries by now. I'd be happy to reverse-engineer some additional commits to make it look like I was more prescient :).
Before I get into specifics, pacman already has "hooks" of a sort: install scripts. I think I would prefer to see support added for install scripts in local db packages than the ability to run arbitrary python code. You could then modify files as needed from post_install(). This would match our existing use of install scripts as hooks and could be run within fakechroot. Anybody else have any thoughts on this?
I really like that idea. The reason I didn't go down that path was that I saw what pmpkg.install_package contained, or rather did not contain, and was scared off.
The only reason to refactor this code into functions seems to be so that you can run pacman from genhook, but you only do that to install packages, which should actually be done by adding them to the local db, so I'm not really sure what the point of this is. Furthermore, I'm not sure how wise it is to use pacman to set up its own test environment.
Testing -Qk(k) requires /var/lib/pacman/local/* to be populated, which is not something pmpkg does. So how much more does pmpkg need to do? The more it duplicates what pacman does, the higher the ongoing code maintenance cost. So where's the sweet spot? I would argue that pmpkg should do the minimum (i.e. only what it currently does). And that there is no danger in using pacman to set up its own test environment because there are extensive unit tests for the installation features. And if those pass, then where is the danger? It might be nice to add "this test should fail without even running unless that test has succeeded" to the specifications, but that seems a bit overkill. One more subtle thing to keep in mind is the timing of the snapshot. Now I didn't really dig deeply into how all the tests make use of the snapshot, but for some tests you want it taken between the install and the mangling. There is no way to achieve this effect using local pmpkgs. But I want to say again that I really like the idea. There's no question it is superior to my hooks mechanism in the complexity of the testing framework and even the generality. I like it so much that I'm going to delay responding to the other points from your code review to provide an opportunity for feedback. I think it should be possible to find workarounds for the issues I mentioned above, and if that's the case then I would happily abandon my hooks idea in favor of install scripts. Thanks again, Jeremy