On 12/16/13 at 01:38pm, Jeremy Heiner wrote:
On Sat, Dec 14, 2013 at 4:03 PM, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
One of the main reasons I suggested adding install script support for this was so that they would work the same as scripts run by pacman and therefore be easy to write and understand. At the very least, they should be run in root, not TMPDIR and honor env["scriptlet-shell"]. This should also be run with fakeroot (use -i and -s to save state between invocations) and be run inside a fake chroot. The only downside I see is the need to copy binaries into the test chroot. It looks like your tests would require us to copy at least chown, chmod, rm, touch, and ln into the chroot. Is there any problem with copying the additional binaries?
Hey Andrew, thanks for the code review! I obviously wrote these patches following the "less is more" principle. Every added line of code is a future potential headache. And if I don't have a solid use-case argument backing up my edits I'm not going to put my name up for blame.
I can get my head around the argument that running scripts from the test root is much less surprising. That gives me what I need to put my name on that additional bit of code. But I don't have enough yet for me to feel comfortable taking responsibility for your other suggestions...
The --scriptlet-shell code makes zero sense to me. The help string in configure doesn't match the implementation. There is a hard-coded ref to '/bin/sh' in pmtest.generate, so it is guaranteed that 'bin/sh' is available under the test root. All --scriptlet-shell does is install a 2nd copy of '/bin/sh' under a different path, and maybe I'm missing something because that seems to accomplish nothing. What is the point of telling the test framework to use an alias? Anyway, I would feel uncomfortable submitting a patch that perpetuates this confusion, and I am unsure of how to resolve it (which would seem best done in its own patch set).
The alias isn't for the test runner, it's for pacman. If it didn't exist, pacman wouldn't be able to find the configured SCRIPTLET_SHELL. I was slightly mistaken in my original comment; which path should be used for local scripts actually depends on whether or not they end up being run in a chroot. Running in a chroot, they would use --scriptlet-shell because we know it will be suitable to use for scripts because pacman itself needs it. Outside a chroot, they should continue using '/bin/sh' because that's what the sync scripts in the tests currently get regardless of what scriptlet-shell refers to.
I can see what the 'fake' commands accomplish for the binaries, but can't figure out a scenario where they would be of use for the test framework. These scripts are run in the phase of a test where the framework unpacks the 'local' stuff in preparation for eventually running the binaries. What commands would a test writer invoke during that phase which would need the 'fake' environment set up? I would feel weird submitting a patch that adds even the small complexity of the 'fake' commands without also submitting a positive test case that demonstrates it does what it is supposed to. And I don't know what that test case would be.
fakeroot will let you call chown in querycheck008. Using a chroot allows us to use --scriptlet-shell, which we know will be suitable for running scripts, instead of 'bin/sh' and improves compatibility with the scripts run by pacman, which can contain absolute paths. Though, paths in tests should generally be relative anyway and I doubt we'll be moving away from using 'bin/sh' for sync scripts anytime soon. The gains are fairly minor, so I'm not terribly concerned about using a chroot.
The downside you mention isn't nothing. Sure, the test filesystem gets built to RAM (typically), so copying all those extra commands for each test is pretty fast. But not copying is still faster. And simpler: there is the ongoing maintenance cost for the list of commands to be copied as new tests are written. Again, these scripts are explicitly written to be invoked during a specific phase, where it is known that a test root is being built, so what advantage is there to having this one little part of the test framework pretend that it doesn't know about the real root? Setting up that pretense just amounts to extra busywork with zero gain.
I'm very willing to be convinced I've got it wrong. My familiarity with the code is increasing, but still a long way from where you are at. And if I don't yet know enough to see the use-case justifying the requested feature then I'm probably not the right one to trust to implement it correctly. The conservative approach seems to me to add only what is required by the current tests. It is always easier to add more complexity as more tests are written than it is to ever yank out code that doesn't seem to do anything but maybe one day will be used. Jeremy