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.

Sign up for GitHub

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

Jump to bottom

WIP: Windows Support #999

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

WIP: Windows Support #999

wants to merge 4 commits into from

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented Sep 7, 2021 •
edited
Loading

Ref #1000

@codecov
Copy link

codecov bot commented Sep 7, 2021 •
edited
Loading

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.02%. Comparing base (10b1fe4) to head (be3f494).
Report is 174 commits behind head on master.

Files with missing lines Patch % Lines
sink.go 42.85% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   98.20%   98.02%   -0.19%     
==========================================
  Files          47       47              
  Lines        2067     2073       +6     
==========================================
+ Hits         2030     2032       +2     
- Misses         29       32       +3     
- Partials        8        9       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was referenced Sep 7, 2021
Open
Open
Merged
abhinav added a commit that referenced this pull request Sep 10, 2021
@abhinav
CI: Test with Go 1.17
32c2f85 
Add Go 1.17 to the list of versions we test with. `gofmt` all files to
add the new `//go:build` directives meant to replace `// +build`.

This supersedes the Go version upgrade in #999. That PR will be narrowed
down just to Windows support.
@abhinav abhinav changed the base branch from master to abg/go117 September 10, 2021 12:43
@abhinav abhinav changed the title CI: Test with Go 1.16, 1.17 on Windows and Linux WIP: Windows Support Sep 10, 2021
abhinav added a commit that referenced this pull request Sep 10, 2021
@abhinav
CI: Test with Go 1.17 (#1009)
a0e2380 
Add Go 1.17 to the list of versions we test with. `gofmt` all files to
add the new `//go:build` directives meant to replace `// +build`.

This supersedes the Go version upgrade in #999. That PR will be narrowed
down just to Windows support.
Base automatically changed from abg/go117 to master September 10, 2021 16:27
@abhinav
CI: Test with Windows
c508650 
Update the GitHub workflow configuration to test on both, Linux and
Windows.
@abhinav abhinav force-pushed the abg/workflow branch 2 times, most recently from 4911015 to 2f204b0 Compare November 20, 2021 00:16
@abhinav
newSink: Support Windows paths
82e841b 
We use `newSink` to decide where the zap.Config.OutputPaths field
intends to send logs. It can be in the form,

    file:///foo/bar
    # or equivalently,
    /foo/bar

Or a user can register a custom sink constructor with `RegisterSink`,
and then use the specified scheme in the output paths.

    zap.RegisterSink("myscheme", ...)

    zap.Config{
        OutputPaths: []string{
            "myscheme://whatever/I/want",
        },
    }

This method of configuration hasn't worked for Windows (ref #994)
because we use `url.Parse` to parse these output paths.

`url.Parse` parses a Windows file path like `C:\foo\bar` to the URL:

    URL{
        Scheme: "c",
        Opaque: `\foo\bar`,
    }

This commit adds support for Windows file paths.
It does so by converting "\" symbols in the output path with "/"
before attempting to parse it with url.Parse. This gives us,

    URL{
        Scheme: "c",
        Path: "/foo/bar",
    }

Following that, we check if the scheme matches the path's "volume name".
On Windows, the volume name of `C:\foo\bar` is `"C:"`.
On Unix, the volume name of all paths is `""`.

This lets us convert the partial file path in the URL
back to a valid Windows file path
and set the scheme to "file" to use the file sink.
@abhinav
Turn to file:// only if no factory match
f551a8a 
If we register a factory with the scheme `m://` on Windows,
matching blindly on just the volume name will turn
`m://foo/bar` into `file://m:\foo\bar`.

We need to do the `file://` conversion only if we didn't find a factory
with the name in the path.
@abhinav
filepath.IsAbs to check for absolute path
be3f494 
`strings.HasPrefix(.., '/')` is Unix specific.
@abhinav abhinav mentioned this pull request Apr 29, 2022
Open
@shvc
Copy link

shvc commented May 20, 2022

any update?

@prashantv prashantv mentioned this pull request Aug 25, 2022
Merged
abhinav pushed a commit that referenced this pull request Sep 12, 2022
@prashantv
Open absolute paths as files, limited Windows support (#1159)
7cabba7 
Ref #994, #1000

Alternate approach to #999, this approach brings absolute path support
to Windows.
It does so by checking for absolute paths and handling them using the
file factory directly.

To test different paths without trying to actually open them (UNC paths
hit the network), this change also
prefactors the sink registry into a separate type that allows stubbing
of the `os.OpenFile` call.

Note: This change does not bring full Windows support -- many tests
still fail, and Windows file URIs don't work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abhinav @shvc