Merge pull request #5761 from hashicorp/fix_5713

fix nasty edge case where we can't find guest additions on windows if they are on a different drive

builder/virtualbox/ovf/config.go

@@ -2,8 +2,6 @@ package ovf
 
 import (
 	"fmt"
-	"net/url"
-	"os"
 	"strings"
 
 	vboxcommon "github.com/hashicorp/packer/builder/virtualbox/common"
@@ -103,14 +101,12 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) {
 		if err != nil {
 			errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is invalid: %s", err))
 		}
-		// file must exist now.
-		fileURL, _ := url.Parse(c.SourcePath)
-		if fileURL.Scheme == "file" {
-			if _, err := os.Stat(fileURL.Path); err != nil {
-				errs = packer.MultiErrorAppend(errs,
-					fmt.Errorf("source file needs to exist at time of config validation: %s", err))
-			}
+		fileOK := common.FileExistsLocally(c.SourcePath)
+		if !fileOK {
+			packer.MultiErrorAppend(errs,
+				fmt.Errorf("Source file needs to exist at time of config validation!"))
 		}
+
 	}
 
 	validMode := false

common/config.go

@@ -50,78 +50,121 @@ func DownloadableURL(original string) (string, error) {
 	if runtime.GOOS == "windows" {
 		// If the distance to the first ":" is just one character, assume
 		// we're dealing with a drive letter and thus a file path.
+		// prepend with "file:///"" now so that url.Parse won't accidentally
+		// parse the drive letter into the url scheme.
+		// See https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/
+		// for more info about valid windows URIs
 		idx := strings.Index(original, ":")
 		if idx == 1 {
 			original = "file:///" + original
 		}
 	}
-
-	url, err := url.Parse(original)
+	u, err := url.Parse(original)
 	if err != nil {
 		return "", err
 	}
 
-	if url.Scheme == "" {
-		url.Scheme = "file"
+	if u.Scheme == "" {
+		u.Scheme = "file"
 	}
 
-	if url.Scheme == "file" {
+	if u.Scheme == "file" {
 		// Windows file handling is all sorts of tricky...
 		if runtime.GOOS == "windows" {
 			// If the path is using Windows-style slashes, URL parses
 			// it into the host field.
-			if url.Path == "" && strings.Contains(url.Host, `\`) {
-				url.Path = url.Host
-				url.Host = ""
-			}
-
-			// For Windows absolute file paths, remove leading / prior to processing
-			// since net/url turns "C:/" into "/C:/"
-			if len(url.Path) > 0 && url.Path[0] == '/' {
-				url.Path = url.Path[1:len(url.Path)]
+			if u.Path == "" && strings.Contains(u.Host, `\`) {
+				u.Path = u.Host
+				u.Host = ""
 			}
 		}
-
 		// Only do the filepath transformations if the file appears
 		// to actually exist.
-		if _, err := os.Stat(url.Path); err == nil {
-			url.Path, err = filepath.Abs(url.Path)
+		if _, err := os.Stat(u.Path); err == nil {
+			u.Path, err = filepath.Abs(u.Path)
 			if err != nil {
 				return "", err
 			}
 
-			url.Path, err = filepath.EvalSymlinks(url.Path)
+			u.Path, err = filepath.EvalSymlinks(u.Path)
 			if err != nil {
 				return "", err
 			}
 
-			url.Path = filepath.Clean(url.Path)
+			u.Path = filepath.Clean(u.Path)
 		}
 
 		if runtime.GOOS == "windows" {
 			// Also replace all backslashes with forwardslashes since Windows
 			// users are likely to do this but the URL should actually only
 			// contain forward slashes.
-			url.Path = strings.Replace(url.Path, `\`, `/`, -1)
+			u.Path = strings.Replace(u.Path, `\`, `/`, -1)
+			// prepend absolute windows paths with "/" so that when we
+			// compose u.String() below the outcome will be correct
+			// file:///c/blah syntax; otherwise u.String() will only add
+			// file:// which is not technically a correct windows URI
+			if filepath.IsAbs(u.Path) && !strings.HasPrefix(u.Path, "/") {
+				u.Path = "/" + u.Path
+			}
+
 		}
 	}
 
 	// Make sure it is lowercased
-	url.Scheme = strings.ToLower(url.Scheme)
+	u.Scheme = strings.ToLower(u.Scheme)
 
 	// Verify that the scheme is something we support in our common downloader.
 	supported := []string{"file", "http", "https"}
 	found := false
 	for _, s := range supported {
-		if url.Scheme == s {
+		if u.Scheme == s {
 			found = true
 			break
 		}
 	}
 
 	if !found {
-		return "", fmt.Errorf("Unsupported URL scheme: %s", url.Scheme)
+		return "", fmt.Errorf("Unsupported URL scheme: %s", u.Scheme)
 	}
-
-	return url.String(), nil
+	return u.String(), nil
+}
+
+// FileExistsLocally takes the URL output from DownloadableURL, and determines
+// whether it is present on the file system.
+// example usage:
+//
+// myFile, err = common.DownloadableURL(c.SourcePath)
+// ...
+// fileExists := common.StatURL(myFile)
+// possible output:
+// true -- should occur if the file is present, or if the file is not present,
+// but is not supposed to be (e.g. the schema is http://, not file://)
+// false -- should occur if there was an error stating the file, so the
+// file is not present when it should be.
+
+func FileExistsLocally(original string) bool {
+	// original should be something like file://C:/my/path.iso
+
+	fileURL, _ := url.Parse(original)
+	fileExists := false
+
+	if fileURL.Scheme == "file" {
+		// on windows, correct URI is file:///c:/blah/blah.iso.
+		// url.Parse will pull out the scheme "file://" and leave the path as
+		// "/c:/blah/blah/iso".  Here we remove this forward slash on absolute
+		// Windows file URLs before processing
+		// see https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/
+		// for more info about valid windows URIs
+		filePath := fileURL.Path
+		if runtime.GOOS == "windows" && len(filePath) > 0 && filePath[0] == '/' {
+			filePath = filePath[1:]
+		}
+		_, err := os.Stat(filePath)
+		if err != nil {
+			return fileExists
+		} else {
+			fileExists = true
+		}
+	}
+	return fileExists
 }

common/config_test.go

@@ -74,6 +74,72 @@ func TestDownloadableURL(t *testing.T) {
 	}
 }
 
+func TestDownloadableURL_WindowsFiles(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		dirCases := []struct {
+			InputString string
+			OutputURL   string
+			ErrExpected bool
+		}{ // TODO: add different directories
+			{
+				"C:\\Temp\\SomeDir\\myfile.txt",
+				"file:///C:/Temp/SomeDir/myfile.txt",
+				false,
+			},
+			{ // need windows drive
+				"\\Temp\\SomeDir\\myfile.txt",
+				"",
+				true,
+			},
+			{ // need windows drive
+				"/Temp/SomeDir/myfile.txt",
+				"",
+				true,
+			},
+			{ // UNC paths; why not?
+				"\\\\?\\c:\\Temp\\SomeDir\\myfile.txt",
+				"",
+				true,
+			},
+			{
+				"file:///C:\\Temp\\SomeDir\\myfile.txt",
+				"file:///c:/Temp/SomeDir/myfile.txt",
+				false,
+			},
+			{
+				"file:///c:/Temp/Somedir/myfile.txt",
+				"file:///c:/Temp/SomeDir/myfile.txt",
+				false,
+			},
+		}
+		// create absolute-pathed tempfile to play with
+		err := os.Mkdir("C:\\Temp\\SomeDir", 0755)
+		if err != nil {
+			t.Fatalf("err creating test dir: %s", err)
+		}
+		fi, err := os.Create("C:\\Temp\\SomeDir\\myfile.txt")
+		if err != nil {
+			t.Fatalf("err creating test file: %s", err)
+		}
+		fi.Close()
+		defer os.Remove("C:\\Temp\\SomeDir\\myfile.txt")
+		defer os.Remove("C:\\Temp\\SomeDir")
+
+		// Run through test cases to make sure they all parse correctly
+		for _, tc := range dirCases {
+			u, err := DownloadableURL(tc.InputString)
+			if (err != nil) != tc.ErrExpected {
+				t.Fatalf("Test Case failed: Expected err = %#v, err = %#v, input = %s",
+					tc.ErrExpected, err, tc.InputString)
+			}
+			if u != tc.OutputURL {
+				t.Fatalf("Test Case failed: Expected %s but received %s from input %s",
+					tc.OutputURL, u, tc.InputString)
+			}
+		}
+	}
+}
+
 func TestDownloadableURL_FilePaths(t *testing.T) {
 	tf, err := ioutil.TempFile("", "packer")
 	if err != nil {
@@ -145,6 +211,43 @@ func TestDownloadableURL_FilePaths(t *testing.T) {
 	}
 }
 
+func test_FileExistsLocally(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		dirCases := []struct {
+			Input  string
+			Output bool
+		}{
+			// file exists locally
+			{"file:///C:/Temp/SomeDir/myfile.txt", true},
+			// file is not supposed to exist locally
+			{"https://myfile.iso", true},
+			// file does not exist locally
+			{"file:///C/i/dont/exist", false},
+		}
+		// create absolute-pathed tempfile to play with
+		err := os.Mkdir("C:\\Temp\\SomeDir", 0755)
+		if err != nil {
+			t.Fatalf("err creating test dir: %s", err)
+		}
+		fi, err := os.Create("C:\\Temp\\SomeDir\\myfile.txt")
+		if err != nil {
+			t.Fatalf("err creating test file: %s", err)
+		}
+		fi.Close()
+		defer os.Remove("C:\\Temp\\SomeDir\\myfile.txt")
+		defer os.Remove("C:\\Temp\\SomeDir")
+
+		// Run through test cases to make sure they all parse correctly
+		for _, tc := range dirCases {
+			fileOK := FileExistsLocally(tc.Input)
+			if !fileOK {
+				t.Fatalf("Test Case failed: Expected %#v, received = %#v, input = %s",
+					tc.Output, fileOK, tc.Input)
+			}
+		}
+	}
+}
+
 func TestScrubConfig(t *testing.T) {
 	type Inner struct {
 		Baz string

common/download.go

@@ -15,6 +15,7 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"runtime"
 )
 
 // DownloadConfig is the configuration given to instantiate a new
@@ -129,6 +130,11 @@ func (d *DownloadClient) Get() (string, error) {
 		// locally and we don't make a copy. Normally we would copy or download.
 		log.Printf("[DEBUG] Using local file: %s", finalPath)
 
+		// Remove forward slash on absolute Windows file URLs before processing
+		if runtime.GOOS == "windows" && len(finalPath) > 0 && finalPath[0] == '/' {
+			finalPath = finalPath[1:]
+		}
+
 		// Keep track of the source so we can make sure not to delete this later
 		sourcePath = finalPath
 		if _, err = os.Stat(finalPath); err != nil {