Handle Windows long file names in a few more places. #1382
Conversation
There was a problem hiding this comment.
@ianthehat has more experience with Windows than I do and should take a second look. This change looks good to me, but I have a few readability comments, and there's a test failure.
So if I understand correctly, in order to be safe on Windows, we need to make every path absolute before calling any file API on Windows? Is this a common problem, or does it affect a few things (protoc) that have particularly long paths?
go/tools/builders/compile.go
Outdated
| @@ -49,6 +49,7 @@ func run(args []string) error { | |||
| if err := goenv.update(); err != nil { | |||
| return err | |||
| } | |||
| absOutput := abs(*output) | |||
There was a problem hiding this comment.
Without any comment here, we'll probably forget why this is here and remove it in six months. :)
It would also be good to have a more detailed comment on abs itself that references path_windows.go as you did in the commit message.
go/tools/builders/protoc.go
Outdated
| info.created = true | ||
|
|
||
| if f.IsDir() { | ||
| if err := os.Mkdir(abs(filepath.Join(*outPath, relPath)), f.Mode()); !os.IsExist(err) { |
There was a problem hiding this comment.
Move the abs call to *outPath next to tmpDir above and use the result here. The comment can explain why both are needed.
go/tools/builders/protoc.go
Outdated
| return err | ||
| } | ||
| tmpDir = abs(tmpDir) | ||
|
|
There was a problem hiding this comment.
defer os.RemoveAll(tmpDir) instead of calling it at the end.
go/tools/builders/protoc.go
Outdated
| path: path, | ||
| base: filepath.Base(path), | ||
| created: true, | ||
| } | ||
| files[path] = info | ||
|
|
||
| foundInfo := files[relPath] |
There was a problem hiding this comment.
if foundInfo, ok := files[relPath]; ok { ...
| @@ -37,7 +37,7 @@ type goMetadata struct { | |||
| func readFiles(bctx build.Context, inputs []string) ([]*goMetadata, error) { | |||
| outputs := []*goMetadata{} | |||
| for _, input := range inputs { | |||
| if m, err := readGoMetadata(bctx, input, true); err != nil { | |||
| if m, err := readGoMetadata(bctx, abs(input), true); err != nil { | |||
There was a problem hiding this comment.
Looks like tests are failing because //go/builders:filter_test does not include env.go, which it now needs.
Go can handle long file names on Windows if they are not relative. This change makes some file paths absolute so that go can handle them. See https://github.com/golang/go/blob/b86e76681366447798c94abb959bb60875bcc856/src/os/path_windows.go#L133. The protoc binary cannot handle long filenames either, so this change generates go protos in the temporary folder and then moves the contents into place.
Yes.
The path length limitation is a Windows global thing. Most tools work around it, by using the |
Go can handle long file names on Windows if they are not relative. This change makes some file paths absolute so that go can handle them. See https://github.com/golang/go/blob/b86e76681366447798c94abb959bb60875bcc856/src/os/path_windows.go#L133. The protoc binary cannot handle long filenames either, so this change generates go protos in the temporary folder and then moves the contents into place.
Go can handle long file names on Windows if they are not relative. This change makes some file paths absolute so that go can handle them. See https://github.com/golang/go/blob/b86e76681366447798c94abb959bb60875bcc856/src/os/path_windows.go#L133.
The
protocbinary cannot handle long filenames either, so this change generates go protos in the temporary folder and then moves the contents into place.