builder/amazon/chroot: CommandWrapper

/cc @mwhooker - I changed the interface up a bit to return an error,
since things should return errors in Go (the ui.Error bit was kind of
ghetto because it had no way to bubble that error up except through the
UI).

Using this, I made it so that the communicator uses both a
CommandWrapper and ShellCommand with chroot so that the chroot commannd
is also wrapped (it wasn't before).

I think the functionality of all this is the same but I'd love if you
could look it over and make sure.

builder/amazon/chroot/builder.go

@@ -13,7 +13,6 @@ import (
 	"github.com/mitchellh/packer/common"
 	"github.com/mitchellh/packer/packer"
 	"log"
-	"os/exec"
 	"runtime"
 )
 
@@ -165,15 +164,11 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe
 
 	ec2conn := ec2.New(auth, region)
 
-	wrappedCommand := func(command string) *exec.Cmd {
-		wrapped, err := b.config.tpl.Process(
+	wrappedCommand := func(command string) (string, error) {
+		return b.config.tpl.Process(
 			b.config.CommandWrapper, &wrappedCommandTemplate{
 				Command: command,
 			})
-		if err != nil {
-			ui.Error(err.Error())
-		}
-		return ShellCommand(wrapped)
 	}
 
 	// Setup the state bag and initial state for the steps
@@ -182,7 +177,7 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe
 	state.Put("ec2", ec2conn)
 	state.Put("hook", hook)
 	state.Put("ui", ui)
-	state.Put("wrappedCommand", Command(wrappedCommand))
+	state.Put("wrappedCommand", CommandWrapper(wrappedCommand))
 
 	// Build the steps
 	steps := []multistep.Step{

builder/amazon/chroot/command.go

@@ -0,0 +1,15 @@
+package chroot
+
+import (
+	"os/exec"
+)
+
+// CommandWrapper is a type that given a command, will possibly modify that
+// command in-flight. This might return an error.
+type CommandWrapper func(string) (string, error)
+
+// ShellCommand takes a command string and returns an *exec.Cmd to execute
+// it within the context of a shell (/bin/sh).
+func ShellCommand(command string) *exec.Cmd {
+	return exec.Command("/bin/sh", "-c", command)
+}

builder/amazon/chroot/communicator.go

@@ -1,6 +1,6 @@
 package chroot
 
-//  pf := func () { somefunc("a str", 1) } 
+//  pf := func () { somefunc("a str", 1) }
 
 import (
 	"fmt"
@@ -14,18 +14,21 @@ import (
 	"syscall"
 )
 
-type Command func(string) *exec.Cmd
-
 // Communicator is a special communicator that works by executing
 // commands locally but within a chroot.
 type Communicator struct {
-	Chroot         string
-	ChrootCmd      Command
-	WrappedCommand Command
+	Chroot     string
+	CmdWrapper CommandWrapper
 }
 
 func (c *Communicator) Start(cmd *packer.RemoteCmd) error {
-	localCmd := c.ChrootCmd(cmd.Command)
+	command, err := c.CmdWrapper(
+		fmt.Sprintf("sudo chroot %s '%s'", c.Chroot, cmd.Command))
+	if err != nil {
+		return err
+	}
+
+	localCmd := ShellCommand(command)
 	localCmd.Stdin = cmd.Stdin
 	localCmd.Stdout = cmd.Stdout
 	localCmd.Stderr = cmd.Stderr
@@ -66,41 +69,25 @@ func (c *Communicator) Upload(dst string, r io.Reader) error {
 	}
 	defer os.Remove(tf.Name())
 	io.Copy(tf, r)
-	cpCmd := fmt.Sprintf("cp %s %s", tf.Name(), dst)
-	return (c.WrappedCommand(cpCmd)).Run()
-}
 
-func (c *Communicator) UploadDir(dst string, src string, exclude []string) error {
-	/*
-		walkFn := func(fullPath string, info os.FileInfo, err error) error {
-			if err != nil {
-				return err
-			}
-
-			path, err := filepath.Rel(src, fullPath)
-			if err != nil {
-				return err
-			}
-
-			for _, e := range exclude {
-				if e == path {
-					log.Printf("Skipping excluded file: %s", path)
-					return nil
-				}
-			}
+	cpCmd, err := c.CmdWrapper(fmt.Sprintf("cp %s %s", tf.Name(), dst))
+	if err != nil {
+		return err
+	}
 
-			chrootDest := filepath.Join(c.Chroot, dst, path)
-			log.Printf("Uploading dir %s to chroot dir: %s", src, dst)
-			cpCmd := fmt.Sprintf("cp %s %s", fullPath, chrootDest)
-			return c.WrappedCommand(cpCmd).Run()
-		}
-	*/
+	return ShellCommand(cpCmd).Run()
+}
 
+func (c *Communicator) UploadDir(dst string, src string, exclude []string) error {
 	// TODO: remove any file copied if it appears in `exclude`
 	chrootDest := filepath.Join(c.Chroot, dst)
 	log.Printf("Uploading directory '%s' to '%s'", src, chrootDest)
-	cpCmd := fmt.Sprintf("cp -R %s* %s", src, chrootDest)
-	return c.WrappedCommand(cpCmd).Run()
+	cpCmd, err := c.CmdWrapper(fmt.Sprintf("cp -R %s* %s", src, chrootDest))
+	if err != nil {
+		return err
+	}
+
+	return ShellCommand(cpCmd).Run()
 }
 
 func (c *Communicator) Download(src string, w io.Writer) error {

builder/amazon/chroot/copy_files.go

@@ -1,19 +1 @@
 package chroot
-
-import (
-	"fmt"
-	"log"
-	"os/exec"
-)
-
-func ChrootCommand(chroot string, command string) *exec.Cmd {
-	cmd := fmt.Sprintf("sudo chroot %s", chroot)
-	return ShellCommand(cmd, command)
-}
-
-func ShellCommand(commands ...string) *exec.Cmd {
-	cmds := append([]string{"-c"}, commands...)
-	cmd := exec.Command("/bin/sh", cmds...)
-	log.Printf("ShellCommand: %s %v", cmd.Path, cmd.Args[1:])
-	return cmd
-}

builder/amazon/chroot/step_chroot_provision.go

@@ -4,7 +4,6 @@ import (
 	"github.com/mitchellh/multistep"
 	"github.com/mitchellh/packer/packer"
 	"log"
-	"os/exec"
 )
 
 // StepChrootProvision provisions the instance within a chroot.
@@ -16,16 +15,12 @@ func (s *StepChrootProvision) Run(state multistep.StateBag) multistep.StepAction
 	hook := state.Get("hook").(packer.Hook)
 	mountPath := state.Get("mount_path").(string)
 	ui := state.Get("ui").(packer.Ui)
-	wrappedCommand := state.Get("wrappedCommand").(Command)
-	chrootCmd := func(command string) *exec.Cmd {
-		return ChrootCommand(mountPath, command)
-	}
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
 
 	// Create our communicator
 	comm := &Communicator{
-		Chroot:         mountPath,
-		ChrootCmd:      chrootCmd,
-		WrappedCommand: wrappedCommand,
+		Chroot:     mountPath,
+		CmdWrapper: wrappedCommand,
 	}
 
 	// Provision

builder/amazon/chroot/step_copy_files.go

@@ -22,7 +22,7 @@ func (s *StepCopyFiles) Run(state multistep.StateBag) multistep.StepAction {
 	config := state.Get("config").(*Config)
 	mountPath := state.Get("mount_path").(string)
 	ui := state.Get("ui").(packer.Ui)
-	wrappedCommand := state.Get("wrappedCommand").(Command)
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
 	stderr := new(bytes.Buffer)
 
 	s.files = make([]string, 0, len(config.CopyFiles))
@@ -33,8 +33,16 @@ func (s *StepCopyFiles) Run(state multistep.StateBag) multistep.StepAction {
 			chrootPath := filepath.Join(mountPath, path)
 			log.Printf("Copying '%s' to '%s'", path, chrootPath)
 
-			cmd := wrappedCommand(fmt.Sprintf("cp %s %s", path, chrootPath))
+			cmdText, err := wrappedCommand(fmt.Sprintf("cp %s %s", path, chrootPath))
+			if err != nil {
+				err := fmt.Errorf("Error building copy command: %s", err)
+				state.Put("error", err)
+				ui.Error(err.Error())
+				return multistep.ActionHalt
+			}
+
 			stderr.Reset()
+			cmd := ShellCommand(cmdText)
 			cmd.Stderr = stderr
 			if err := cmd.Run(); err != nil {
 				err := fmt.Errorf(
@@ -60,11 +68,16 @@ func (s *StepCopyFiles) Cleanup(state multistep.StateBag) {
 }
 
 func (s *StepCopyFiles) CleanupFunc(state multistep.StateBag) error {
-	wrappedCommand := state.Get("wrappedCommand").(Command)
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
 	if s.files != nil {
 		for _, file := range s.files {
 			log.Printf("Removing: %s", file)
-			localCmd := wrappedCommand(fmt.Sprintf("rm -f %s", file))
+			localCmdText, err := wrappedCommand(fmt.Sprintf("rm -f %s", file))
+			if err != nil {
+				return err
+			}
+
+			localCmd := ShellCommand(localCmdText)
 			if err := localCmd.Run(); err != nil {
 				return err
 			}

builder/amazon/chroot/step_mount_device.go

@@ -27,6 +27,7 @@ func (s *StepMountDevice) Run(state multistep.StateBag) multistep.StepAction {
 	config := state.Get("config").(*Config)
 	ui := state.Get("ui").(packer.Ui)
 	device := state.Get("device").(string)
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
 
 	mountPath, err := config.tpl.Process(config.MountPath, &mountPathData{
 		Device: filepath.Base(device),
@@ -58,9 +59,16 @@ func (s *StepMountDevice) Run(state multistep.StateBag) multistep.StepAction {
 
 	ui.Say("Mounting the root device...")
 	stderr := new(bytes.Buffer)
-	mountCommand := fmt.Sprintf("mount %s %s", device, mountPath)
-	wrappedCommand := state.Get("wrappedCommand").(Command)
-	cmd := wrappedCommand(mountCommand)
+	mountCommand, err := wrappedCommand(
+		fmt.Sprintf("mount %s %s", device, mountPath))
+	if err != nil {
+		err := fmt.Errorf("Error creating mount command: %s", err)
+		state.Put("error", err)
+		ui.Error(err.Error())
+		return multistep.ActionHalt
+	}
+
+	cmd := ShellCommand(mountCommand)
 	cmd.Stderr = stderr
 	if err := cmd.Run(); err != nil {
 		err := fmt.Errorf(
@@ -91,11 +99,15 @@ func (s *StepMountDevice) CleanupFunc(state multistep.StateBag) error {
 	}
 
 	ui := state.Get("ui").(packer.Ui)
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
+
 	ui.Say("Unmounting the root device...")
+	unmountCommand, err := wrappedCommand(fmt.Sprintf("umount %s", s.mountPath))
+	if err != nil {
+		return fmt.Errorf("Error creating unmount command: %s", err)
+	}
 
-	unmountCommand := fmt.Sprintf("umount %s", s.mountPath)
-	wrappedCommand := state.Get("wrappedCommand").(Command)
-	cmd := wrappedCommand(unmountCommand)
+	cmd := ShellCommand(unmountCommand)
 	if err := cmd.Run(); err != nil {
 		return fmt.Errorf("Error unmounting root device: %s", err)
 	}

builder/amazon/chroot/step_mount_extra.go

@@ -20,7 +20,7 @@ func (s *StepMountExtra) Run(state multistep.StateBag) multistep.StepAction {
 	config := state.Get("config").(*Config)
 	mountPath := state.Get("mount_path").(string)
 	ui := state.Get("ui").(packer.Ui)
-	wrappedCommand := state.Get("wrappedCommand").(Command)
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
 
 	s.mounts = make([]string, 0, len(config.ChrootMounts))
 
@@ -42,12 +42,19 @@ func (s *StepMountExtra) Run(state multistep.StateBag) multistep.StepAction {
 
 		ui.Message(fmt.Sprintf("Mounting: %s", mountInfo[2]))
 		stderr := new(bytes.Buffer)
-		mountCommand := fmt.Sprintf(
+		mountCommand, err := wrappedCommand(fmt.Sprintf(
 			"mount %s %s %s",
 			flags,
 			mountInfo[1],
-			innerPath)
-		cmd := wrappedCommand(mountCommand)
+			innerPath))
+		if err != nil {
+			err := fmt.Errorf("Error creating mount command: %s", err)
+			state.Put("error", err)
+			ui.Error(err.Error())
+			return multistep.ActionHalt
+		}
+
+		cmd := ShellCommand(mountCommand)
 		cmd.Stderr = stderr
 		if err := cmd.Run(); err != nil {
 			err := fmt.Errorf(
@@ -78,15 +85,18 @@ func (s *StepMountExtra) CleanupFunc(state multistep.StateBag) error {
 		return nil
 	}
 
-	wrappedCommand := state.Get("wrappedCommand").(Command)
+	wrappedCommand := state.Get("wrappedCommand").(CommandWrapper)
 	for len(s.mounts) > 0 {
 		var path string
 		lastIndex := len(s.mounts) - 1
 		path, s.mounts = s.mounts[lastIndex], s.mounts[:lastIndex]
-		unmountCommand := fmt.Sprintf("umount %s", path)
+		unmountCommand, err := wrappedCommand(fmt.Sprintf("umount %s", path))
+		if err != nil {
+			return fmt.Errorf("Error creating unmount command: %s", err)
+		}
 
 		stderr := new(bytes.Buffer)
-		cmd := wrappedCommand(unmountCommand)
+		cmd := ShellCommand(unmountCommand)
 		cmd.Stderr = stderr
 		if err := cmd.Run(); err != nil {
 			return fmt.Errorf(