Repository: gobuffalo/buffalo
Pull request #1294 (2018-09-07 18:18:47)

Do not join absolute paths on build messages
`buffalo build` was showing the wrong output message when -o was provided with
an absolute path because it was appending it to b.Root when it shouldn't be doing
that.

Example of how it was:

```
$ buffalo build -o /tmp/test
You application was successfully built at /Users/alex/go/src/github.com/agonzalezro/buffalo/tmp/test
```

Example of how it is:

```
$ buffalo build -o /tmp/test
You application was successfully built at /tmp/test
```

agonzalezro: I was trying to make the test more flexible using `path.Join` for the expected result instead writing it down, however I think it make it difficult to understand. What do you think about having a test for Windows systems and one for *nix system using build tags for this?
markbates: you could use `filepath.Join` in the tests instead of `foo/bar`. I haven't looked at the errors, but I'm assuming its that based on the test code.
agonzalezro: The "problem" if the tests are not separated for Windows and *nix is that an input binary path like this: `/foo/bar` in Windows should end up (I think, please correct me if I am wrong) in something like: `C:\\thisismycurrentpathexample\/foo/bar`. Instead, in *nix that would be an absolute path and it would end up at `/foo/bar`.

If we don't want to add logic to the tests the cleanest approach seems to have two different tests cases depending on the OS. I was asking the question because I saw build tags only for appengine and integration tests and maybe you don't want to follow that approach and I am overcomplicating something that should be simpler.
markbates: Ah gotcha. We haven’t been using build tags for testing like that , but have been using runtime.GOOS for that check and just having two different tables in the same test. That way we don’t have to change the test logic but just the data we feed in/out.

--
Mark Bates
On Sep 22, 2018, 6:03 PM -0400, Alexandre González <notifications@github.com>, wrote:
> The "problem" if the tests are not separated for Windows and *nix is that an input binary path like this: /foo/bar in Windows should end up (I think, please correct me if I am wrong) in something like: C:\\thisismycurrentpathexample\/foo/bar. Instead, in *nix that would be an absolute path and it would end up at /foo/bar.
> If we don't want to add logic to the tests the cleanest approach seems to have two different tests cases depending on the OS. I was asking the question because I saw build tags only for appengine and integration tests and maybe you don't want to follow that approach and I am overcomplicating something that should be simpler.
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.


Patch:
diff --git a/buffalo/cmd/build.go b/buffalo/cmd/build.go
index b1642475..ff250926 100644
--- a/buffalo/cmd/build.go
+++ b/buffalo/cmd/build.go
@@ -3,7 +3,6 @@ package cmd
 import (
 	"context"
 	"os"
-	"path/filepath"
 
 	"github.com/gobuffalo/buffalo/buffalo/cmd/build"
 	"github.com/gobuffalo/buffalo/meta"
@@ -56,8 +55,7 @@ var xbuildCmd = &cobra.Command{
 			return errors.WithStack(err)
 		}
 
-		bin, _ := filepath.Abs(b.Bin)
-		logrus.Infof("\nYour application was successfully built at %s\n", bin)
+		logrus.Infof("\nYour application was successfully built at %s\n", b.AbsoluteBinaryPath())
 
 		return nil
 	},
diff --git a/buffalo/cmd/build/path.go b/buffalo/cmd/build/path.go
new file mode 100644
index 00000000..4f6628f6
--- /dev/null
+++ b/buffalo/cmd/build/path.go
@@ -0,0 +1,11 @@
+package build
+
+import "path/filepath"
+
+// AbsoluteBinaryPath returns the absolute path to the binary.
+func (b *Builder) AbsoluteBinaryPath() string {
+	if filepath.IsAbs(b.Bin) {
+		return b.Bin
+	}
+	return filepath.Join(b.Root, b.Bin)
+}


Test patch:
diff --git a/buffalo/cmd/build/path_test.go b/buffalo/cmd/build/path_test.go
new file mode 100644
index 00000000..0730f0bd
--- /dev/null
+++ b/buffalo/cmd/build/path_test.go
@@ -0,0 +1,60 @@
+package build
+
+import (
+	"runtime"
+	"testing"
+
+	"github.com/gobuffalo/buffalo/meta"
+	"github.com/stretchr/testify/assert"
+)
+
+const rootPath = "ROOT"
+
+func TestAbsoluteBinaryPath(t *testing.T) {
+	tests := []struct {
+		inBinaryPath                             string
+		expectedNixResult, expectedWindowsResult string
+	}{
+		// relative paths on *nix
+		{"relnixbinary",
+			rootPath + "/relnixbinary", rootPath + "\\relnixbinary"},
+		{"rel/nix/binary",
+			rootPath + "/rel/nix/binary", rootPath + "\\rel\\nix\\binary"},
+
+		// relative paths on Windows
+		{"relwinbinary",
+			rootPath + "/relwinbinary", rootPath + "\\relwinbinary"},
+		{"rel\\win\\binary",
+			rootPath + "/rel\\win\\binary", rootPath + "\\rel\\win\\binary"},
+
+		// absolute paths on *nix
+		{"/absnixbinary",
+			"/absnixbinary", rootPath + "\\absnixbinary"},
+		{"/abs/nix/binary",
+			"/abs/nix/binary", rootPath + "\\abs\\nix\\binary"},
+
+		// absolute paths on Windows
+		{"C:\\abswinbinary",
+			rootPath + "/C:\\abswinbinary", "C:\\abswinbinary"},
+		{"C:\\abs\\win\\binary",
+			rootPath + "/C:\\abs\\win\\binary", "C:\\abs\\win\\binary"},
+	}
+
+	for _, test := range tests {
+		b := Builder{
+			Options: Options{
+				App: meta.App{
+					Root: rootPath,
+					Bin:  test.inBinaryPath,
+				},
+			},
+		}
+		if runtime.GOOS == "windows" {
+			assert.Equal(t, test.expectedWindowsResult, b.AbsoluteBinaryPath())
+		} else {
+			assert.Equal(t, test.expectedNixResult, b.AbsoluteBinaryPath())
+
+		}
+
+	}
+}