Skip to content

Compile on Windows #65

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
Closed
lowenna wants to merge 1 commit into from
Closed

Compile on Windows #65

lowenna wants to merge 1 commit into from

Conversation

@lowenna
Copy link

@lowenna lowenna commented Apr 14, 2017

Signed-off-by: John Howard jhoward@microsoft.com

@stevvooe @dmcgowan

I need this to compile cleanly on Windows for containerd. Fixups only - I don't know how functional it is yet. Verified it builds clean for windows, darwin and linux

E:\go\src\github.com\stevvooe\continuity [compile-on-windows]> $env:GOOS="windows"
E:\go\src\github.com\stevvooe\continuity [compile-on-windows]> go build
E:\go\src\github.com\stevvooe\continuity [compile-on-windows]> $env:GOOS="linux"
E:\go\src\github.com\stevvooe\continuity [compile-on-windows]> go build
E:\go\src\github.com\stevvooe\continuity [compile-on-windows]> $env:GOOS="darwin"
E:\go\src\github.com\stevvooe\continuity [compile-on-windows]> go build
E:\go\src\github.com\stevvooe\continuity [compile-on-windows]>

And verified unit tests pass on Linux:

jhoward@jhoward-z420:[/mnt/e/go/src/github.com/stevvooe/continuity]: $ go test -v
=== RUN   TestUniqifyDigests
--- PASS: TestUniqifyDigests (0.00s)
=== RUN   TestWalkFS
--- PASS: TestWalkFS (0.34s)
        manifest_test.go:291: drwx------ /tmp/continuity-test-282077333
        manifest_test.go:291: -rw-r--r-- /tmp/continuity-test-282077333/a
        manifest_test.go:291: -rw-r--r-- /tmp/continuity-test-282077333/a-hardlink
        manifest_test.go:291: drwxr-xr-x /tmp/continuity-test-282077333/b
        manifest_test.go:291: trw------- /tmp/continuity-test-282077333/b/a
        manifest_test.go:291: -rw-r--r-- /tmp/continuity-test-282077333/b/a-hardlink
        manifest_test.go:291: drwxr-xr-x /tmp/continuity-test-282077333/c
        manifest_test.go:291: -rw-r--r-- /tmp/continuity-test-282077333/c/a
        manifest_test.go:289: Lrwxrwxrwx /tmp/continuity-test-282077333/c/a-abssymlink -> /tmp/continuity-test-282077333/a
        manifest_test.go:289: Lrwxrwxrwx /tmp/continuity-test-282077333/c/a-relsymlink -> ../a
        manifest_test.go:289: Lrwxrwxrwx /tmp/continuity-test-282077333/c/ca-relsymlink -> a
        manifest_test.go:291: drwxr-xr-x /tmp/continuity-test-282077333/dev
        manifest_test.go:291: prw-rw-rw- /tmp/continuity-test-282077333/fifo
        manifest_test.go:303:
                /tmp/continuity-test-282077333
                |-- a
                |-- a-hardlink
                |-- b
                |   |-- a
                |   `-- a-hardlink
                |-- c
                |   |-- a
                |   |-- a-abssymlink -> /tmp/continuity-test-282077333/a
                |   |-- a-relsymlink -> ../a
                |   `-- ca-relsymlink -> a
                |-- dev
                `-- fifo
        manifest_test.go:146: resource: <
                  path: "/a"
                  path: "/a-hardlink"
                  path: "/b/a-hardlink"
                  uid: "1000"
                  gid: "1000"
                  mode: 420
                  size: 2458145
                  digest: "sha256:7fc322fdc391b682d92cd446c437de5b68b921f63546e57677b6dde20c69338a"
                >
                resource: <
                  path: "/b"
                  uid: "1000"
                  gid: "1000"
                  mode: 2147484141
                >
                resource: <
                  path: "/b/a"
                  uid: "1000"
                  gid: "1000"
                  mode: 1048960
                  size: 2725329
                  digest: "sha256:c14fd9031d44df50744fee6d893bff8609212962903971823d3a24b27aa2b872"
                >
                resource: <
                  path: "/c"
                  uid: "1000"
                  gid: "1000"
                  mode: 2147484141
                >
                resource: <
                  path: "/c/a"
                  uid: "1000"
                  gid: "1000"
                  mode: 420
                  size: 573728
                  digest: "sha256:e75dcb2e1625dcb0a80015e79d8e59fb05b42df29bd2460461a7ae3b2dc3cfd8"
                >
                resource: <
                  path: "/c/a-abssymlink"
                  uid: "1000"
                  gid: "1000"
                  mode: 134218239
                  target: "/tmp/continuity-test-282077333/a"
                >
                resource: <
                  path: "/c/a-relsymlink"
                  uid: "1000"
                  gid: "1000"
                  mode: 134218239
                  target: "../a"
                >
                resource: <
                  path: "/c/ca-relsymlink"
                  uid: "1000"
                  gid: "1000"
                  mode: 134218239
                  target: "a"
                >
                resource: <
                  path: "/dev"
                  uid: "1000"
                  gid: "1000"
                  mode: 2147484141
                >
                resource: <
                  path: "/fifo"
                  uid: "1000"
                  gid: "1000"
                  mode: 33554870
                >

PASS
ok      github.com/stevvooe/continuity  0.429s
jhoward@jhoward-z420:[/mnt/e/go/src/github.com/stevvooe/continuity]: $

Note that the unit tests have not been ported to Windows yet and don't pass.

@stevvooe
Copy link
Member

stevvooe commented Apr 14, 2017

LGTM

Any chance we can get the unit tests working? :p

@lowenna
Copy link
Author

lowenna commented Apr 14, 2017

I plan to, but figured containerd is slightly more important to make progress on.... Hold fire on merging - looks like I missed some formatting on a file.

@lowenna lowenna force-pushed the compile-on-windows branch 5 times, most recently from c1349e2 to 36478f2 Compare April 22, 2017 04:08
Compile on Windows
281ab83 
Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna lowenna force-pushed the compile-on-windows branch from 36478f2 to 281ab83 Compare April 22, 2017 04:10
@lowenna
Copy link
Author

lowenna commented Apr 22, 2017

There was a really weird gofmt problem with manifest_test.go which I can't explain, but seems fixed now. Also noted that resource.go was failing the gofmt test, even though travis-ci appeared to complete successfully. Regardless - fixed now.

stevvooe
stevvooe reviewed Apr 24, 2017
View reviewed changes
context_windows.go
import "os"

// getUidGidFromFileInfo extracts the user and group IDs from a fileinfo.
// This is Unix-specific functionality.
Copy link
Member

@stevvooe stevvooe Apr 24, 2017

Choose a reason for hiding this comment

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

Is there still not an owner and group in msft? We made these strings specifically to be compatible here. Seems odd removing it. How will this data be recovered when used with continuity?

Copy link
Author

@lowenna lowenna Apr 24, 2017

Choose a reason for hiding this comment

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

UID/GID are very Unix specific. Windows uses ACES in ACLS (https://msdn.microsoft.com/en-us/library/windows/desktop/aa374872(v=vs.85).aspx). Certainly as a longer-term plan, this needs to be considered. However, I wish to re-iterate this is just to get this component to compile on Windows, not to functionally prove it. It allows forward progress on containerd.

Copy link
Member

@stevvooe stevvooe Apr 24, 2017

Choose a reason for hiding this comment

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

Do we not foresee putting ACEs in uid/gid? If not, can we revert these fields to integers?

On other systems, I have seen these as strings since they can be used for the ACE identifier, but I may be missing something here.

Copy link
Author

@lowenna lowenna Apr 24, 2017

Choose a reason for hiding this comment

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

The easiest way to encode an ACL is in SDDL which is essentially a string. See https://msdn.microsoft.com/en-us/library/windows/desktop/aa379567(v=vs.85).aspx. You can see an example of SDDL in the docker code base such as https://github.com/moby/moby/blob/master/pkg/system/filesys_windows.go#L99-L120.

Calling them a uid or gid is really not appropriate on Windows.

Copy link
Member

@stevvooe stevvooe Apr 24, 2017

Choose a reason for hiding this comment

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

@jhowardmsft Sounds like we'll need another field...

@stevvooe
Copy link
Member

stevvooe commented Apr 24, 2017

LGTM

@jhowardmsft It looks like a few things are stubbed out. It would be great to have a plan to figure out how we can pick up a windows filesystem from a lossy archive and continuity manifest and get the same result back. Ignoring the ownership seems to be unexpected from the goals of this project.

@lowenna
Copy link
Author

lowenna commented Apr 24, 2017

@stevvooe Closing anyway. Looks like @dmcgowan has gone through the same exercise and that got merged.

@lowenna lowenna closed this Apr 24, 2017
@stevvooe
Copy link
Member

stevvooe commented Apr 24, 2017

@stevvooe Closing anyway. Looks like @dmcgowan has gone through the same exercise and that got merged.

Woops, Sorry. Let me know if we need changes.

@lowenna lowenna mentioned this pull request May 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevvooe stevvooe stevvooe left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lowenna @stevvooe