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

Windows paths not supported by default file sink #621

Open
Apeopex opened this issue Aug 13, 2018 · 16 comments
Open

Windows paths not supported by default file sink #621

Apeopex opened this issue Aug 13, 2018 · 16 comments

Comments

@Apeopex
Copy link

Apeopex commented Aug 13, 2018

When using absolute Windows paths, url.Parse does not retreieve the "file" scheme.

panic: couldn't open sink "C:\\Users\\...\\AppData\\Roaming\\...\\info.log.json": no sink found for scheme "c"

Appending, "file:///" (golang/go#6027 (comment)) does not work as expected. (Ref: golang/go#13276 )

zap/sink.go

Line 99 in ff33455

u, err := url.Parse(rawURL)

Ref: golang/go#13276 (comment)
Changed by PR: #606

Temporary workaround:

func newWinFileSink(u *url.URL) (zap.Sink, error) {
    // Remove leading slash left by url.Parse()
    return os.OpenFile(u.Path[1:], os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
}
zap.RegisterSink("winfile", newWinFileSink)
zap.Config{
    OutputPaths: []string{"stdout", "winfile:///" + filepath.Join(GlobalConfigDir.Path, "info.log.json")},
}
@wenzhihong2003
Copy link

yes, in windows file:///d:/a/wen88.log is not supported. Because, in this scene, url.Path is /d:/a/wen88.log, in windows you can't create file start with /, so in windows, you will strip the /.

@wenzhihong2003
Copy link

I have pull a request in #624

@prashantv
Copy link
Collaborator

The reason file:///d:/a/wen88.log doesn't work is that the path is /d:/a/..., can you replace the 3 / with 2 /, e.g., file://d:/a/wen88.log?

@Apeopex
Copy link
Author

Apeopex commented Aug 23, 2018

I did try that and ran into the same issue listed here: golang/go#13276

With backslashes:
panic: couldn't open sink "file://C:\\Users\\~~\\Documents\\Web Development\\~~\\build\\info.log.json": can't parse "file://C:\\Users\\~~\\Documents\\Web Development\\~~\\build\\info.log.json" as a URL: parse file://C:\Users\~~\Documents\Web Development\~~\build\info.log.json: invalid character "\\" in host name

With forward slashes:
panic: couldn't open sink "file://C:/Users/~~/Documents/Web Development/~~/build/info.log.json": file URLs must leave host empty or use localhost: got file://C:/Users/~~/Documents/Web%20Development/~~/build/info.log.json

@wenzhihong2003
Copy link

wenzhihong2003 commented Aug 24, 2018

@prashantv , if use file://d:/a/wen88.log , it will paic with message

couldn't open sink "file://d:/a/wen88.log": file URLs must leave host empty or use localhost: got file://d:/a/wen88.log

because, file://d:/a/wen88.log parse to url.URL,

Host = "d:"
Path = "/a/wen88.log"

it will panic in https://github.com/uber-go/zap/blob/master/sink.go#L130

if hn := u.Hostname(); hn != "" && hn != "localhost" {
    return nil, fmt.Errorf("file URLs must leave host empty or use localhost: got %v", u)
}

@prashantv
Copy link
Collaborator

That makes sense, thanks. It looks like it has to be 3 slashes, and is covered here:
https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/

Is there any other examples of Go libraries that parse Windows file URIs?

@sethkor
Copy link

sethkor commented May 19, 2019

Any update on this one, i'm running into this issue right now on windows

@whatsadebugger
Copy link

I just had the same problem. Might need to look into dimrocs pr and add those changes manually.

@whatsadebugger
Copy link

smartcontractkit/chainlink#977 copy logger_unix and _windows. Call the the registerOSSinks method to register the sink and you're good to go.

@izikorgad
Copy link

+1

@Againster
Copy link

+1, it still didn't work

@Mario-Hofstaetter
Copy link

Dear maintainers, 3 years later this issue is still open and is breaking logging in https://github.com/open-telemetry/opentelemetry-collector

Is there any workaround yet that does not involve source changes? (which is not possible because we use otelcol release)

PR #624 was unfortunately never finished.

@abhinav
Copy link
Collaborator

abhinav commented Apr 29, 2022

@Mario-Hofstaetter
Unfortunately, this has been a pretty low priority issue because we don't use Windows at work, and none of the active maintainers have Windows machines at home to be able to debug and fix this.

My feeling is that is that some mix of filepath.ToSlash/FromSlash will help solve this (I made an attempt in #999 last year), but the edit-test-debug loop was slow enough that I stopped trying.

prashantv added a commit to prashantv/zap that referenced this issue Aug 16, 2022
This is a prefactor for Windows support (uber-go#621).

`TestOpen` currently relies on a hardcoded error "no such file or
directory" if a file is not found. However, this error message is
OS-specific.  We can now rely on `errors.Is(err, fs.ErrNotExist)`
if we wrap errors using `%w` instead of `%v`.
abhinav added a commit that referenced this issue Aug 16, 2022
This is a prefactor for Windows support (#621).
TestOpen currently relies on a hardcoded error "no such file or
directory" if a file is not found. However, this error message is
OS-specific.  We can now rely on `errors.Is(err, fs.ErrNotExist)`
if we wrap errors using %w instead of %v.

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
@kmahyyg
Copy link

kmahyyg commented Nov 22, 2022

@Mario-Hofstaetter Unfortunately, this has been a pretty low priority issue because we don't use Windows at work, and none of the active maintainers have Windows machines at home to be able to debug and fix this.

My feeling is that is that some mix of filepath.ToSlash/FromSlash will help solve this (I made an attempt in #999 last year), but the edit-test-debug loop was slow enough that I stopped trying.

I understand that this issue is low-priority since you never use Windows at work. Golang is shipped with cross-platform ability, this issue really make users exhausted.

@prashantv
Copy link
Collaborator

There is some limited Windows support as of #1159

I think that change should have made v1.24 (@sywhang I don't see it in the release changelog, my PR didn't update the CHANGELOG, so it may have been missed?)

@yehudamakarov
Copy link

funny, my zap logger is breaking on windows and i guess this is why.

Is the below still the recommended fix?

When using absolute Windows paths, url.Parse does not retreieve the "file" scheme.

panic: couldn't open sink "C:\\Users\\...\\AppData\\Roaming\\...\\info.log.json": no sink found for scheme "c"

Appending, "file:///" (golang/go#6027 (comment)) does not work as expected. (Ref: golang/go#13276 )

zap/sink.go

Line 99 in ff33455

u, err := url.Parse(rawURL)

Ref: golang/go#13276 (comment) Changed by PR: #606

Temporary workaround:

func newWinFileSink(u *url.URL) (zap.Sink, error) {
    // Remove leading slash left by url.Parse()
    return os.OpenFile(u.Path[1:], os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
}
zap.RegisterSink("winfile", newWinFileSink)
zap.Config{
    OutputPaths: []string{"stdout", "winfile:///" + filepath.Join(GlobalConfigDir.Path, "info.log.json")},
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests