On 12/16/13 at 01:38pm, Jeremy Heiner wrote: > On Sat, Dec 14, 2013 at 4:03 PM, Andrew Gregory > <[email protected]> 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
