beehive icon indicating copy to clipboard operation
beehive copied to clipboard

MS Windows fixes

Open rubiojr opened this issue 4 years ago • 10 comments

  • 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 https://github.com/golang/go/issues/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 avatar May 09 '20 18:05 rubiojr

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

rubiojr avatar May 09 '20 19:05 rubiojr

@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.

rubiojr avatar May 09 '20 19:05 rubiojr

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 avatar May 10 '20 13:05 rubiojr

This is looking good now.

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

rubiojr avatar May 10 '20 14:05 rubiojr

Fixed in https://github.com/muesli/beehive/pull/310/commits/602668adc400c0a5ef2465d1fe1080ef1099e060

rubiojr avatar May 10 '20 14:05 rubiojr

@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.

muesli avatar Oct 03 '20 15:10 muesli

sounds good!

rubiojr avatar Oct 03 '20 15:10 rubiojr

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

rubiojr avatar Oct 10 '20 14:10 rubiojr

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

penguwin avatar Oct 16 '20 10:10 penguwin

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

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.

rubiojr avatar Oct 25 '20 12:10 rubiojr