Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some path logic to work on Windows #2704

Merged
merged 29 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c18b611
Fix some path logic to work on Windows
jchadwick-buf Jan 9, 2024
85f9e39
Set a FSRoot in filepathext package
doriable Jan 10, 2024
4d88b2a
Run `make generate`
doriable Jan 10, 2024
a106cf0
Small fixes to test
doriable Jan 11, 2024
1f88005
Some more fixes
doriable Jan 11, 2024
b5c666e
Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…
doriable Jan 11, 2024
ffdf1ef
More reader_test assertion fixes
doriable Jan 11, 2024
34ce447
Need to parse components in order to get relative input path
doriable Jan 11, 2024
c64f727
Fix absolute path tests for Windows workspaces
doriable Jan 11, 2024
3df5d92
Attempt to use USERPROFILE to get volume of current process
doriable Jan 11, 2024
6003241
Revert back to using normalpath.Components fix and add comments
doriable Jan 11, 2024
3ac2242
Fix some low hanging fruit to unnormalize file paths in error returns
doriable Jan 11, 2024
07041e7
Add filepath.Clean to decode/encode errors
doriable Jan 11, 2024
fd39088
A couple more clean-ups
doriable Jan 11, 2024
c93de2f
More fixes
doriable Jan 11, 2024
fa317be
More simple fixes
doriable Jan 11, 2024
0caa8ed
Add TODOs for archive test failures
doriable Jan 16, 2024
7334382
Change exclude path error
doriable Jan 16, 2024
51e85b3
Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…
doriable Jan 16, 2024
ebe2579
Add skips for formatter and bufimageutil/generate tests
doriable Jan 16, 2024
ea950f7
Add skips for storagetesting package for carriage returns
doriable Jan 16, 2024
37740e7
Add more TODOs
doriable Jan 16, 2024
1fe3539
Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…
doriable Jan 17, 2024
2bc8859
Change the .gitattributes to `-text -diff` for testdata files
doriable Jan 17, 2024
8ecf668
Ungate bufimageutil tests
doriable Jan 17, 2024
66a0f04
Ungate generate test
doriable Jan 17, 2024
bb6c42f
Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…
doriable Jan 17, 2024
c5b8cb9
Use t.TempDir() for archive tests for unique temp directories for tes…
doriable Jan 17, 2024
d8924ce
Skip invalid path archive tests
doriable Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions private/buf/buffetch/internal/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/pkg/app"
"github.com/bufbuild/buf/private/pkg/filepathext"
"github.com/bufbuild/buf/private/pkg/git"
"github.com/bufbuild/buf/private/pkg/httpauth"
"github.com/bufbuild/buf/private/pkg/ioext"
Expand Down Expand Up @@ -573,7 +574,7 @@ func getReadWriteBucketForOS(
return nil, err
}
osRootBucket, err := storageosProvider.NewReadWriteBucket(
string(os.PathSeparator),
filepathext.FSRoot,
// TODO: is this right? verify in deleted code
storageos.ReadWriteBucketWithSymlinksIfSupported(),
)
Expand All @@ -584,8 +585,9 @@ func getReadWriteBucketForOS(
ctx,
logger,
osRootBucket,
// This makes the path relative to the bucket.
absInputDirPath[1:],
// This makes the path relative to the bucket. We need to strip away the root of the
// absolute path.
normalpath.Join(normalpath.Components(absInputDirPath)[1:]...),
terminateFunc,
)
if err != nil {
Expand All @@ -606,7 +608,7 @@ func getReadWriteBucketForOS(
// Make bucket on: os.PathSeparator + returnMapPath (since absolute)
var bucketPath string
if filepath.IsAbs(normalpath.Unnormalize(inputDirPath)) {
bucketPath = normalpath.Join("/", mapPath)
bucketPath = normalpath.Join(filepathext.FSRoot, mapPath)
} else {
pwd, err := osext.Getwd()
if err != nil {
Expand Down Expand Up @@ -674,7 +676,7 @@ func getReadBucketCloserForOSProtoFile(
return nil, err
}
osRootBucket, err := storageosProvider.NewReadWriteBucket(
"/",
filepathext.FSRoot,
// TODO: is this right? verify in deleted code
storageos.ReadWriteBucketWithSymlinksIfSupported(),
)
Expand All @@ -687,8 +689,9 @@ func getReadBucketCloserForOSProtoFile(
ctx,
logger,
osRootBucket,
// This makes the path relative to the bucket.
absProtoFileDirPath[1:],
// This makes the path relative to the bucket. We need to strip away the root of the
// absolute path.
normalpath.Join(normalpath.Components(absProtoFileDirPath)[1:]...),
protoFileTerminateFunc,
)
if err != nil {
Expand Down Expand Up @@ -736,7 +739,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(filepathext.FSRoot, mapPath)
} else {
pwd, err := osext.Getwd()
if err != nil {
Expand Down Expand Up @@ -861,8 +864,8 @@ func attemptToFixOSRootBucketPathErrors(err error) error {
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(filepathext.FSRoot, pathError.Path), normalpath.Absolute) {
relPath, err := normalpath.Rel(pwd, normalpath.Join(filepathext.FSRoot, pathError.Path))
// Just ignore if this errors and do nothing.
if err == nil {
// Making a copy just to be super-safe.
Expand Down
34 changes: 17 additions & 17 deletions private/buf/buffetch/internal/reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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())
}

Expand All @@ -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())
}

Expand Down Expand Up @@ -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())
}

Expand Down Expand Up @@ -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())
}

Expand All @@ -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())
}

Expand Down Expand Up @@ -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())
}

Expand Down
4 changes: 2 additions & 2 deletions private/buf/cmd/buf/workspace_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestWorkspaceAbsoluteFail(t *testing.T) {
nil,
1,
``,
`Failure: directory "C:\buf" listed in testdata\workspace\fail\absolute\windows\buf.work.yaml is invalid: C:\buf: expected to be relative`,
`Failure: decode testdata/workspace/fail/absolute/windows/buf.work.yaml: C:\buf: expected to be relative`,
"build",
filepath.Join("testdata", "workspace", "fail", "absolute", "windows"),
)
Expand All @@ -38,7 +38,7 @@ func TestWorkspaceAbsoluteFail(t *testing.T) {
nil,
1,
``,
`Failure: directory "C:\buf" listed in testdata\workspace\fail\absolute\windows\buf.work.yaml is invalid: C:\buf: expected to be relative`,
`Failure: decode testdata/workspace/fail/absolute/windows/buf.work.yaml: C:\buf: expected to be relative`,
"build",
filepath.Join("testdata", "workspace", "fail", "v2", "absolute", "windows"),
)
Expand Down
26 changes: 26 additions & 0 deletions private/pkg/filepathext/filepathext_unix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2020-2023 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build aix || darwin || dragonfly || freebsd || (js && wasm) || linux || netbsd || openbsd || solaris
// +build aix darwin dragonfly freebsd js,wasm linux netbsd openbsd solaris

package filepathext

import (
"os"
)

var (
FSRoot = string(os.PathSeparator)
)
30 changes: 30 additions & 0 deletions private/pkg/filepathext/filepathext_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2020-2023 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build windows
// +build windows

package filepathext

import (
"os"
"path/filepath"
)

var (
// USERPROFILE shows the current user's profile folder. It is being used to parse the volume
// for the current user and this is used as the process's context.
// https://learn.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables#variables-that-are-processed-for-the-operating-system-and-in-the-context-of-each-user
FSRoot = filepath.VolumeName(os.Getenv("USERPROFILE")) + string(os.PathSeparator)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I retract my comment earlier, I think it isn't feasible to parse this through the system env vars -- the process could just be running in an entirely separate drive, which is what is happening in the actions. We should use normalpath.Components when parsing the absolute paths and document clearly the behaviours of how reader.go is constructing the OS root buckets.

)
Loading