On 10/04/13 at 01:18pm, Jeremy Heiner wrote: > On Fri, Oct 4, 2013 at 11:22 AM, Andrew Gregory > <[email protected]> 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?
pmdb is responsible for populating /var/lib/pacman/local/. In fact, you'll need to update pmdb so it will create the mtree file, which I seem to have neglected to mention before. > 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. The snapshot is used to determine if the file was modified after being installed. None of your tests needed a post-snapshot hook, and I can't think of any that would. Of course, if such a need does arise, we can revisit the idea of post-snapshot hooks at that time. > 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
