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

MS Windows fixes #310

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

MS Windows fixes #310

wants to merge 14 commits into from

Conversation

rubiojr
Copy link
Collaborator

@rubiojr rubiojr commented May 9, 2020

  • Handle Windows path arguments like c:/path/to/beehive.conf when using --config.
  • Handle crypto URLs correctly
  • Test fixes, making sure we always use forward slashes in paths under Windows.
  • c:\foo\bar windows paths aren't accepted (use forward slash instead)

For some extra context, Go URL parsing in Windows can be a bit surprising.

Parsing file:///c:/beehive.conf, the URI format documented here,
will set the URL path to /c:/beehive.conf.
This behaviour is captured in golang/go#6027.

If we use a URL like file://c:/beehive.conf, the drive name will be parsed
as the host and the path will miss the drive name.

package main

import (
        "fmt"
        "net/url"
)

func main() {
        u, _ := url.Parse(`file://c:/beehive.conf`)
        fmt.Println(u.Host)
        fmt.Println(u.Path)
}

This prints:

c:
/beehive.conf

We workaround it patching the URL path and host so URLs like
crypt://sercret@c:/beehive.conf behave as expected.

@rubiojr
Copy link
Collaborator Author

rubiojr commented May 9, 2020

I think I can come up with a slightly cleaner fix so I don't have to patch every single backend method.

@rubiojr
Copy link
Collaborator Author

rubiojr commented May 9, 2020

@muesli if you don't entirely dislike this patch, we could merge as is to fix the windows situation and I'll work on a new PR, to clean things up a bit.

@@ -106,7 +105,7 @@ func main() {
log.Fatalf("Error creating the configuration %s", err)
}

if config.URL().String() != cfg.DefaultPath() { // the user specified a custom config path or URI
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

String() does some encoding when using Windows paths which is not what we want here.

@rubiojr
Copy link
Collaborator Author

rubiojr commented May 10, 2020

This is looking good now.

Ended up wrapping net/url URL with a drop-in replacement that helps to encapsulate our platform specific logic to patch URLs in Windows.
The wrapper is heavily exercised buy our current tests so I didn't consider necessary to add dedicated unit tests for it.

Also took the chance to add some test helpers and clean things up a bit.

@rubiojr
Copy link
Collaborator Author

rubiojr commented May 10, 2020

This is looking good now.

Not yet, just found one more windows niggle when executing the binary.

@rubiojr
Copy link
Collaborator Author

rubiojr commented May 10, 2020

Fixed in 602668a

@muesli
Copy link
Owner

muesli commented Oct 3, 2020

@rubiojr Just a heads up, @penguwin and I have been pondering how to handle the config URL situation properly for a while and have come up with a few fixes. I think we'll probably have to combine our approaches.

@rubiojr
Copy link
Collaborator Author

rubiojr commented Oct 3, 2020

sounds good!

@penguwin penguwin self-requested a review October 7, 2020 15:48
Copy link
Collaborator

@penguwin penguwin left a comment

Choose a reason for hiding this comment

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

Hey @rubiojr o/

Sorry for responding this late - I've somehow completely missed your PR on this.

I implemented a configuration system in knoxite based on your awesome work in beehive. I'm also about to contribute back some tweaks and enhancements I made in knoxite to beehives config system. As @muesli already addressed I also think it's best to somehow combine both systems - or at least don't let them diverge that much - so we can push improvements and fixes to both programs.

In knoxite the same issues regarding windows paths occurred, however we've used another approach to resolve them. We wrapped a method called PathToUrl around url.Parse(), called by the configs setURL() method. This method should be able to parse all kind of different formatted config file paths (unix/windows) without converting backslashes with a fixWindowsPath method.

Comment on lines +51 to +54
if runtime.GOOS == "windows" {
u.Path = filepath.Join(u.Host, u.Path)
u.Host = ""
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The URL Host probably also needs to be joined for other operating systems too, e.g
crypto://pw@local/folder/beehive.conf gets saved at /folder/beehive.conf

@rubiojr
Copy link
Collaborator Author

rubiojr commented Oct 10, 2020

Awesome @penguwin, let me have a look at the implementation there and I'll get back to you ASAP.

@penguwin
Copy link
Collaborator

cool, if there's anything I can help/assist you with let me know

* Handle Windows path arguments like `c:/path/to/beehive.conf` when using --config.
* Handle crypto URLs correctly
* Test fixes, making sure we always use forward slashes in paths under Windows.

For some extra context, Go URL parsing in Windows can be a bit surprising.

Parsing file:///c:/beehive.conf, the URI format documented [here][1],
will set the URL path to "/c:/beehive.conf".
This behaviour is captured in golang/go#6027.

If we use a URL like file://c:/beehive.conf, the drive name will be parsed
as the host and the path will miss the drive name.

```go
package main

import (
        "fmt"
        "net/url"
)

func main() {
        u, _ := url.Parse(`file://c:/beehive.conf`)
        fmt.Println(u.Host)
        fmt.Println(u.Path)
}
```

This prints:

```
c:
/beehive.conf
```

We workaround it patching the URL path and host so URLs like
`crypt://sercret@c:/beehive.conf` behave as expected.

[1]: https://docs.microsoft.com/en-us/archive/blogs/ie/file-uris-in-windows
Wraps platform-specific workarounds.
@rubiojr
Copy link
Collaborator Author

rubiojr commented Oct 25, 2020

@penguwin: rebased the branch but not without some 😅, since I forgot what I did here in May 😄 .

It'll need some extra polish before asking for a reviewing it again, so I'm taking care of that first.

If y'all haven't started it, I'll also be looking into extracting the configuration system into it's own repo, see if we can make that work for both beehive and knoxite, to avoid duplicated work and the tedious maintenance task of cherry picking fixes for it from one project to the other.

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

Successfully merging this pull request may close these issues.

3 participants