Compile and run on Windows #186

dil-spayne · 2020-09-07T21:37:54Z

This will allow Baur to compile and run on Windows.

I chose cmd as the Windows command shell as it is the base shell for Windows. If users would like to use PowerShell instead they can invoke PowerShell from the cmd shell.
Usages of path have been replaced with filepath. From the Go documentation Package filepath implements utility routines for manipulating filename paths in a way compatible with the target operating system-defined file paths.
filesys_windows.go has been updated to use the newer SecurityAttributes struct, see Move to upstream Sddl/SecurityAttribute functions.

I feel like these changes can be made independently from actually building a Windows binary at this time.
I plan on making the required changes to the Makefile to produce the Windows binary and updating the CI process to run the test cases in due time.

fho

Thanks for the PR.

Please undo the changes to the vendored files, as described in the comments.

There are more occurrences of using functions of the path instead of the filepath package when working with filesystem paths in the code.
Don't they all need to be replaced?

I feel like these changes can be made independently from actually building a Windows binary at this time.

Yes, we can do that in a follow-up step.
Before offering windows binaries we also need to run all tests also on Windows in CI.

fho · 2020-09-08T12:18:01Z

vendor/github.com/docker/docker/pkg/system/filesys_windows.go

@@ -103,13 +102,13 @@ func mkdirall(path string, applyACL bool, sddl string) error {
 // and Local System.
 func mkdirWithACL(name string, sddl string) error {
 	sa := windows.SecurityAttributes{Length: 0}
-	sd, err := winio.SddlToSecurityDescriptor(sddl)
+	sd, err := windows.SecurityDescriptorFromString(sddl)


The file of vendored libraries must never be changed. They must be in the same state then the upstream library in the same version.

If the change is in the upstream library please update it via go get and go mod vendor.

If it is not, we have to try to get the change into the upstream version. In the worst case the library needs to be forked.

Can we postpone the change until the upstream library was updated? What will break without the change?

Thanks for explaining this to me. As mentioned I'm new to Go and did find it a bit strange that the vendor files are committed into the repo compared to how other languages work (e.g. the dist directory isn't normally committed when using npm).
It appears that the required change has been made in the upstream library, I'll work towards getting the module reference updated.

It appears that the upstream library has the required change but an updated package has not yet been released.
I will close this PR until github.com/docker/docker is released with this change.

Thanks for explaining this to me. As mentioned I'm new to Go and did find it a bit strange that the vendor files are committed into the repo compared to how other languages work (e.g. the dist directory isn't normally committed when using npm).

By default they aren't. Only the versions and checksums of 3. party libraries are listed in the go.mod and go.sum files.

I prefer to create the offline copies of the libraries, to not have dependencies to 3. party services.
If github and the go module proxies became unavailable or the references are deleted, it would not be possible to build baur anymore.
It also speeds up CI tasks a bit, if the GOCACHE is not shared between all steps :-).

It appears that the upstream library has the required change but an updated package has not yet been released.
I will close this PR until github.com/docker/docker is released with this change.

Could you explain to me why the change is a must have?
Without it, it's not possible to upload docker images at all?

Sorry about the lack of response. I'm looking at supporting baur on Windows again now.
baur does not compile on Windows without this change

$ go build # github.com/docker/docker/pkg/system vendor\github.com\docker\docker\pkg\system\filesys_windows.go:112:24: cannot use uintptr(unsafe.Pointer(&sd[0])) (type uintptr) as type *"golang.org/x/sys/windows".SECURITY_DESCRIPTOR in assignment

Ok, best might be switching to using the official docker client instead of fsouze/go-dockerclient (#189).

All other issues that were fixed in this PR should already be in master. The filepath functions are used, Commands are run directly not via /bin/sh anymore.

Having the Commands run directly has been great so far, thanks for making that change 💯

Unfortunately the issue resides in the official docker client, not in fsouze/go-dockerclient.
This describes the breaking change to the golang.org/x/sys module golang/go#34610
The fix (moby/moby#40021) was merged into the official docker client master branch late last year but has not been released yet. It appears it will be a part of v20 and has not been back-ported into any of the older versions.

There are 2 options that I can think of:

Wait until the official docker client has been released with the fix

Pin the golang.org/x/sys module to the version before the breaking change was made. Rancher have made this change (pin golang.org/x/sys due to breaking API change rancher/rke#2204).

I have got baur compiling with option 2 and am working through the initial test failures on Windows

fho · 2020-09-08T13:17:42Z

vendor/github.com/simplesurance/baur/app.go

@@ -191,7 +191,7 @@ func NewApp(repository *Repository, cfgPath string) (*App, error) {
 			cfgPath)
 	}

-	appAbsPath := path.Dir(cfgPath)
+	appAbsPath := filepath.Dir(cfgPath)


Same here, we can not change vendored files.

Instead we have to it the following way:

I created a branch based on baur 0.18.0 to replace path pkg uses with using the functions from the filepath package instead: 5951bd9.

Please double-check if it contains all needed changes.
If it looks fine to you, I'll create a baur 0.18.1 release.

Then we update the reference of the vendored github.com/simplesurance/baur version to 0.18.1 in your branch.

Even though I plan on closing this PR until the github.com/docker/docker module is updated with the required changes it would still be helpful to create the 0.18.1 release with the path -> filepath changes.

https://github.com/simplesurance/baur/releases/tag/v0.18.1

dil-spayne · 2020-09-08T21:02:29Z

There are more occurrences of using functions of the path instead of the filepath package when working with filesystem paths in the code.
Don't they all need to be replaced?

I was trying to limit the amount of change to only what was required to getting Baur running on Windows. I'm more than happy to update any other references to path with filepath.

dil-spayne · 2020-09-08T22:00:22Z

Closing until github.com/docker/docker is released with newer SecurityAttributes struct support.

fho · 2020-09-15T10:33:04Z

@StephenDiligent we could also fix the path vs filepath import issues separately from the docker package issue.
I created a ticket for it: #187
Feel free to create another PR for it.

This fixes issues when running baur on non-unix operation systems related pr: #186

Compile and run on Windows

6625396

fho requested changes Sep 8, 2020

View reviewed changes

dil-spayne closed this Sep 8, 2020

fho mentioned this pull request Sep 15, 2020

Make filepath operations OS independent #187

Closed

fho added a commit that referenced this pull request Sep 15, 2020

update baur0 dependency to version 0.18.1

e338530

This fixes issues when running baur on non-unix operation systems related pr: #186

fho mentioned this pull request Sep 15, 2020

update baur0 dependency to version 0.18.1 #188

Merged

fho added a commit that referenced this pull request Sep 15, 2020

update baur0 dependency to version 0.18.1

d0e6c16

This fixes issues when running baur on non-unix operation systems related pr: #186

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile and run on Windows #186

Compile and run on Windows #186

dil-spayne commented Sep 7, 2020 •

edited

Loading

fho left a comment

fho Sep 8, 2020

dil-spayne Sep 8, 2020

dil-spayne Sep 8, 2020

fho Sep 15, 2020

dil-spayne Oct 12, 2020

fho Oct 12, 2020

dil-spayne Oct 12, 2020

fho Sep 8, 2020

dil-spayne Sep 8, 2020

fho Sep 15, 2020

dil-spayne commented Sep 8, 2020

dil-spayne commented Sep 8, 2020

fho commented Sep 15, 2020

Compile and run on Windows #186

Compile and run on Windows #186

Conversation

dil-spayne commented Sep 7, 2020 • edited Loading

fho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dil-spayne commented Sep 8, 2020

dil-spayne commented Sep 8, 2020

fho commented Sep 15, 2020

dil-spayne commented Sep 7, 2020 •

edited

Loading