Fix some path logic to work on Windows

private/buf/buffetch/internal/reader.go

@@ -572,8 +572,11 @@ func getReadWriteBucketForOS(
 	if err != nil {
 		return nil, err
 	}
+	// Split the absolute path into components to get the FS root
+	absInputDirPathComponents := normalpath.Components(absInputDirPath)
+	fsRoot := absInputDirPathComponents[0]
 	osRootBucket, err := storageosProvider.NewReadWriteBucket(
-		string(os.PathSeparator),
+		fsRoot,
 		// TODO: is this right? verify in deleted code
 		storageos.ReadWriteBucketWithSymlinksIfSupported(),
 	)
@@ -584,12 +587,11 @@ func getReadWriteBucketForOS(
 		ctx,
 		logger,
 		osRootBucket,
-		// This makes the path relative to the bucket.
-		absInputDirPath[1:],
+		normalpath.Join(absInputDirPathComponents[1:]...), // relative path to the FS root
 		terminateFunc,
 	)
 	if err != nil {
-		return nil, attemptToFixOSRootBucketPathErrors(err)
+		return nil, attemptToFixOSRootBucketPathErrors(fsRoot, err)
 	}
 	// Examples:
 	//
@@ -603,10 +605,10 @@ func getReadWriteBucketForOS(
 	// terminateFileLocation: /users/alice/path/to
 	// returnMapPath: /users/alice/path/to
 	// returnSubDirPath: foo
-	// Make bucket on: os.PathSeparator + returnMapPath (since absolute)
+	// Make bucket on: FS root + returnMapPath (since absolute)
 	var bucketPath string
 	if filepath.IsAbs(normalpath.Unnormalize(inputDirPath)) {
-		bucketPath = normalpath.Join("/", mapPath)
+		bucketPath = normalpath.Join(fsRoot, mapPath)
 	} else {
 		pwd, err := osext.Getwd()
 		if err != nil {
@@ -673,8 +675,11 @@ func getReadBucketCloserForOSProtoFile(
 	if err != nil {
 		return nil, err
 	}
+	// Split the absolute path into components to get the FS root
+	absProtoFileDirPathComponents := normalpath.Components(absProtoFileDirPath)
+	fsRoot := absProtoFileDirPathComponents[0]
 	osRootBucket, err := storageosProvider.NewReadWriteBucket(
-		"/",
+		fsRoot,
 		// TODO: is this right? verify in deleted code
 		storageos.ReadWriteBucketWithSymlinksIfSupported(),
 	)
@@ -687,12 +692,11 @@ func getReadBucketCloserForOSProtoFile(
 		ctx,
 		logger,
 		osRootBucket,
-		// This makes the path relative to the bucket.
-		absProtoFileDirPath[1:],
+		normalpath.Join(absProtoFileDirPathComponents[1:]...), // relative path to the FS root
 		protoFileTerminateFunc,
 	)
 	if err != nil {
-		return nil, attemptToFixOSRootBucketPathErrors(err)
+		return nil, attemptToFixOSRootBucketPathErrors(fsRoot, err)
 	}
 
 	var protoTerminateFileDirPath string
@@ -736,7 +740,7 @@ func getReadBucketCloserForOSProtoFile(
 		// We found a buf.yaml or buf.work.yaml, use that directory.
 		// If we found a buf.yaml or buf.work.yaml and the ProtoFileRef path is absolute, use an absolute path, otherwise relative.
 		if filepath.IsAbs(normalpath.Unnormalize(protoFileDirPath)) {
-			protoTerminateFileDirPath = normalpath.Join("/", mapPath)
+			protoTerminateFileDirPath = normalpath.Join(fsRoot, mapPath)
 		} else {
 			pwd, err := osext.Getwd()
 			if err != nil {
@@ -853,16 +857,16 @@ func getMapPathAndSubDirPath(
 // our attempt to fix that.
 //
 // This is going to take away other intermediate errors unfortunately.
-func attemptToFixOSRootBucketPathErrors(err error) error {
+func attemptToFixOSRootBucketPathErrors(fsRoot string, err error) error {
 	var pathError *fs.PathError
 	if errors.As(err, &pathError) {
 		pwd, err := osext.Getwd()
 		if err != nil {
 			return err
 		}
 		pwd = normalpath.Normalize(pwd)
-		if normalpath.EqualsOrContainsPath(pwd, normalpath.Join("/", pathError.Path), normalpath.Absolute) {
-			relPath, err := normalpath.Rel(pwd, normalpath.Join("/", pathError.Path))
+		if normalpath.EqualsOrContainsPath(pwd, normalpath.Join(fsRoot, pathError.Path), normalpath.Absolute) {
+			relPath, err := normalpath.Rel(pwd, normalpath.Join(fsRoot, pathError.Path))
 			// Just ignore if this errors and do nothing.
 			if err == nil {
 				// Making a copy just to be super-safe.

private/buf/buffetch/internal/reader_test.go

@@ -120,7 +120,7 @@ func TestGetReadWriteBucketForOSNoTerminateFileName(t *testing.T) {
 	require.Equal(t, ".", readWriteBucket.SubDirPath())
 	fileInfo, err := readWriteBucket.Stat(ctx, "buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 }
 
 func TestGetReadWriteBucketForOSTerminateFileName(t *testing.T) {
@@ -137,10 +137,10 @@ func TestGetReadWriteBucketForOSTerminateFileName(t *testing.T) {
 	require.Equal(t, "four/five", readWriteBucket.SubDirPath())
 	fileInfo, err := readWriteBucket.Stat(ctx, "four/five/buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readWriteBucket.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/bufyaml/one/two/three/buf.work.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/bufyaml/one/two/three/buf.work.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 }
 
 func TestGetReadWriteBucketForOSParentPwd(t *testing.T) {
@@ -168,10 +168,10 @@ func TestGetReadWriteBucketForOSParentPwd(t *testing.T) {
 	require.Equal(t, "four/five", readWriteBucket.SubDirPath())
 	fileInfo, err := readWriteBucket.Stat(ctx, "four/five/buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "five/buf.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "five/buf.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readWriteBucket.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "../buf.work.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "../buf.work.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 }
 
 func TestGetReadWriteBucketForOSAbsPwd(t *testing.T) {
@@ -201,10 +201,10 @@ func TestGetReadWriteBucketForOSAbsPwd(t *testing.T) {
 	require.Equal(t, "four/five", readWriteBucket.SubDirPath())
 	fileInfo, err := readWriteBucket.Stat(ctx, "four/five/buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/four/five/buf.yaml"), fileInfo.ExternalPath())
+	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/four/five/buf.yaml"), filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readWriteBucket.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/buf.work.yaml"), fileInfo.ExternalPath())
+	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/buf.work.yaml"), filepath.ToSlash(fileInfo.ExternalPath()))
 }
 
 func TestGetReadBucketCloserForOSProtoFileNoWorkspaceTerminateFileName(t *testing.T) {
@@ -222,7 +222,7 @@ func TestGetReadBucketCloserForOSProtoFileNoWorkspaceTerminateFileName(t *testin
 	require.Equal(t, ".", readBucketCloser.SubDirPath())
 	fileInfo, err := readBucketCloser.Stat(ctx, "buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	require.NoError(t, readBucketCloser.Close())
 }
 
@@ -241,10 +241,10 @@ func TestGetReadBucketCloserForOSProtoFileTerminateFileName(t *testing.T) {
 	require.Equal(t, "four/five", readBucketCloser.SubDirPath())
 	fileInfo, err := readBucketCloser.Stat(ctx, "four/five/buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/bufyaml/one/two/three/four/five/buf.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readBucketCloser.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/bufyaml/one/two/three/buf.work.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/bufyaml/one/two/three/buf.work.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	require.NoError(t, readBucketCloser.Close())
 }
 
@@ -274,10 +274,10 @@ func TestGetReadBucketCloserForOSProtoFileParentPwd(t *testing.T) {
 	require.Equal(t, "four/five", readBucketCloser.SubDirPath())
 	fileInfo, err := readBucketCloser.Stat(ctx, "four/five/buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "five/buf.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "five/buf.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readBucketCloser.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "../buf.work.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "../buf.work.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	require.NoError(t, readBucketCloser.Close())
 }
 
@@ -309,10 +309,10 @@ func TestGetReadBucketCloserForOSProtoFileAbsPwd(t *testing.T) {
 	require.Equal(t, "four/five", readBucketCloser.SubDirPath())
 	fileInfo, err := readBucketCloser.Stat(ctx, "four/five/buf.yaml")
 	require.NoError(t, err)
-	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/four/five/buf.yaml"), fileInfo.ExternalPath())
+	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/four/five/buf.yaml"), filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readBucketCloser.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/buf.work.yaml"), fileInfo.ExternalPath())
+	require.Equal(t, normalpath.Join(absDirPath, "testdata/bufyaml/one/two/three/buf.work.yaml"), filepath.ToSlash(fileInfo.ExternalPath()))
 	require.NoError(t, readBucketCloser.Close())
 }
 
@@ -331,7 +331,7 @@ func TestGetReadBucketCloserForOSProtoFileNoBufYAMLTerminateFileName(t *testing.
 	require.Equal(t, ".", readBucketCloser.SubDirPath())
 	fileInfo, err := readBucketCloser.Stat(ctx, "four/five/proto/foo.proto")
 	require.NoError(t, err)
-	require.Equal(t, "testdata/nobufyaml/one/two/three/four/five/proto/foo.proto", fileInfo.ExternalPath())
+	require.Equal(t, "testdata/nobufyaml/one/two/three/four/five/proto/foo.proto", filepath.ToSlash(fileInfo.ExternalPath()))
 	require.NoError(t, readBucketCloser.Close())
 }
 
@@ -361,10 +361,10 @@ func TestGetReadBucketCloserForOSProtoFileNoBufYAMLParentPwd(t *testing.T) {
 	require.Equal(t, ".", readBucketCloser.SubDirPath())
 	fileInfo, err := readBucketCloser.Stat(ctx, "four/five/proto/foo.proto")
 	require.NoError(t, err)
-	require.Equal(t, "five/proto/foo.proto", fileInfo.ExternalPath())
+	require.Equal(t, "five/proto/foo.proto", filepath.ToSlash(fileInfo.ExternalPath()))
 	fileInfo, err = readBucketCloser.Stat(ctx, "buf.work.yaml")
 	require.NoError(t, err)
-	require.Equal(t, "../buf.work.yaml", fileInfo.ExternalPath())
+	require.Equal(t, "../buf.work.yaml", filepath.ToSlash(fileInfo.ExternalPath()))
 	require.NoError(t, readBucketCloser.Close())
 }
 

private/buf/bufworkspace/option.go

@@ -17,6 +17,7 @@ package bufworkspace
 import (
 	"errors"
 	"fmt"
+	"path/filepath"
 
 	"github.com/bufbuild/buf/private/pkg/normalpath"
 	"github.com/bufbuild/buf/private/pkg/slicesext"
@@ -217,7 +218,10 @@ func newWorkspaceBucketConfig(options []WorkspaceBucketOption) (*workspaceBucket
 				// This is new post-refactor. Before, we gave precedence to --path. While a change,
 				// doing --path foo/bar --exclude-path foo seems like a bug rather than expected behavior to maintain.
 				if normalpath.EqualsOrContainsPath(targetExcludePath, targetPath, normalpath.Relative) {
-					return nil, fmt.Errorf("excluded path %q contains targeted path %q, which means all paths in %q will be excluded", targetExcludePath, targetPath, targetPath)
+					// We clean and unnormalize the target paths to show in the error message
+					unnormalizedTargetExcludePath := normalpath.Unnormalize(filepath.Clean(targetExcludePath))
+					unnormalizedTargetPath := normalpath.Unnormalize(filepath.Clean(targetPath))
+					return nil, fmt.Errorf("excluded path %q contains targeted path %q, which means all paths in %q will be excluded", unnormalizedTargetExcludePath, unnormalizedTargetPath, unnormalizedTargetPath)
 				}
 			}
 		}

private/buf/cmd/buf/command/generate/generate_windows_test.go

@@ -76,8 +76,14 @@ func TestOutputWithExclude(t *testing.T) {
 
 func TestOutputWithPathWithinExclude(t *testing.T) {
 	tempDirPath := t.TempDir()
-	testRunSuccess(
+	testRunStdoutStderr(
 		t,
+		nil,
+		1,
+		``,
+		// This is new post-refactor. Before, we gave precedence to --path. While a change,
+		// doing --path foo/bar --exclude-path foo seems like a bug rather than expected behavior to maintain.
+		filepath.FromSlash(`Failure: excluded path "testdata/paths/a" contains targeted path "testdata/paths/a/v1/a.proto", which means all paths in "testdata/paths/a/v1/a.proto" will be excluded`),
 		"--output",
 		tempDirPath,
 		"--template",
@@ -87,11 +93,6 @@ func TestOutputWithPathWithinExclude(t *testing.T) {
 		"--exclude-path",
 		filepath.Join("testdata", "paths", "a"),
 	)
-
-	_, err := os.Stat(filepath.Join(tempDirPath, "java", "a", "v1", "A.java"))
-	require.NoError(t, err)
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v2", "A.java"))
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
 }
 
 func TestOutputWithExcludeWithinPath(t *testing.T) {
@@ -119,8 +120,14 @@ func TestOutputWithExcludeWithinPath(t *testing.T) {
 
 func TestOutputWithNestedExcludeAndTargetPaths(t *testing.T) {
 	tempDirPath := t.TempDir()
-	testRunSuccess(
+	testRunStdoutStderr(
 		t,
+		nil,
+		1,
+		``,
+		// This is new post-refactor. Before, we gave precedence to --path. While a change,
+		// doing --path foo/bar --exclude-path foo seems like a bug rather than expected behavior to maintain.
+		filepath.FromSlash(`Failure: excluded path "a/v3" contains targeted path "a/v3/foo", which means all paths in "a/v3/foo" will be excluded`),
 		"--output",
 		tempDirPath,
 		"--template",
@@ -133,29 +140,18 @@ func TestOutputWithNestedExcludeAndTargetPaths(t *testing.T) {
 		filepath.Join("testdata", "paths", "a", "v3", "foo"),
 		filepath.Join("testdata", "paths"),
 	)
-	_, err := os.Stat(filepath.Join(tempDirPath, "java", "a", "v3", "foo", "FooOuterClass.java"))
-	require.NoError(t, err)
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "b", "v1", "B.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v1", "A.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v2", "A.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v3", "A.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the file specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v3", "foo", "BarOuterClass.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the file specified.")
 }
 
 func TestWorkspaceGenerateWithExcludeAndTargetPaths(t *testing.T) {
 	tempDirPath := t.TempDir()
-	testRunSuccess(
+	testRunStdoutStderr(
 		t,
+		nil,
+		1,
+		``,
+		// This is new post-refactor. Before, we gave precedence to --path. While a change,
+		// doing --path foo/bar --exclude-path foo seems like a bug rather than expected behavior to maintain.
+		filepath.FromSlash(`Failure: excluded path "a/v3" contains targeted path "a/v3/foo", which means all paths in "a/v3/foo" will be excluded`),
 		"--output",
 		tempDirPath,
 		"--template",
@@ -168,23 +164,5 @@ func TestWorkspaceGenerateWithExcludeAndTargetPaths(t *testing.T) {
 		filepath.Join("testdata", "workspace", "a", "v3", "foo"),
 		"--exclude-path",
 		filepath.Join("testdata", "workspace", "b", "v1", "foo.proto"),
-		filepath.Join("testdata", "workspace"),
 	)
-	_, err := os.Stat(filepath.Join(tempDirPath, "java", "a", "v3", "foo", "FooOuterClass.java"))
-	require.NoError(t, err)
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "b", "v1", "B.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v1", "A.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v2", "A.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the path specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v3", "A.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the file specified.")
-	_, err = os.Stat(filepath.Join(tempDirPath, "java", "a", "v3", "foo", "BarOuterClass.java"))
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "The system cannot find the file specified.")
 }

private/buf/cmd/buf/workspace_test.go

@@ -1086,7 +1086,8 @@ func TestWorkspaceJumpContextFail(t *testing.T) {
 		nil,
 		1,
 		``,
-		filepath.FromSlash(`Failure: decode testdata/workspace/fail/jumpcontext/buf.work.yaml: directory "../breaking/other/proto" is invalid: ../breaking/other/proto: is outside the context directory`),
+		// TODO: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
+		`Failure: decode testdata/workspace/fail/jumpcontext/buf.work.yaml: directory "../breaking/other/proto" is invalid: ../breaking/other/proto: is outside the context directory`,
 		"build",
 		filepath.Join("testdata", "workspace", "fail", "jumpcontext"),
 	)
@@ -1109,7 +1110,8 @@ func TestWorkspaceDirOverlapFail(t *testing.T) {
 		nil,
 		1,
 		``,
-		filepath.FromSlash(`Failure: decode testdata/workspace/fail/diroverlap/buf.work.yaml: directory "foo" contains directory "foo/bar"`),
+		// TODO: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
+		`Failure: decode testdata/workspace/fail/diroverlap/buf.work.yaml: directory "foo" contains directory "foo/bar"`,
 		"build",
 		filepath.Join("testdata", "workspace", "fail", "diroverlap"),
 	)
@@ -1149,7 +1151,8 @@ func TestWorkspaceNoVersionFail(t *testing.T) {
 		nil,
 		1,
 		``,
-		filepath.FromSlash(`Failure: decode testdata/workspace/fail/noversion/buf.work.yaml: "version" is not set. Please add "version: v1"`),
+		// TODO: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
+		`Failure: decode testdata/workspace/fail/noversion/buf.work.yaml: "version" is not set. Please add "version: v1"`,
 		"build",
 		filepath.Join("testdata", "workspace", "fail", "noversion"),
 	)
@@ -1163,7 +1166,8 @@ func TestWorkspaceInvalidVersionFail(t *testing.T) {
 		nil,
 		1,
 		``,
-		filepath.FromSlash(`Failure: decode testdata/workspace/fail/invalidversion/buf.work.yaml: unknown file version: "v9"`),
+		// TODO: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
+		`Failure: decode testdata/workspace/fail/invalidversion/buf.work.yaml: unknown file version: "v9"`,
 		"build",
 		filepath.Join("testdata", "workspace", "fail", "invalidversion"),
 	)
@@ -1177,7 +1181,8 @@ func TestWorkspaceNoDirectoriesFail(t *testing.T) {
 		nil,
 		1,
 		``,
-		filepath.FromSlash(`Failure: decode testdata/workspace/fail/nodirectories/buf.work.yaml: directories is empty`),
+		// TODO: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
+		`Failure: decode testdata/workspace/fail/nodirectories/buf.work.yaml: directories is empty`,
 		"build",
 		filepath.Join("testdata", "workspace", "fail", "nodirectories"),
 	)