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

Fix non-Unix build of libcontainer/user #1509

Closed
wants to merge 3 commits into from
Closed

Conversation

ijc
Copy link
Contributor

@ijc ijc commented Jul 6, 2017

Commit 3d7cb42 ("Move libcontainer to x/sys/unix") switched from
syscall.Get[ug]id to unix.Get[ug]id where unix.* is (naturally) not
available on non-Unix platforms such as Windows.

Move the offending code into a Unix specific file and provide stubs in the
_unsupported.go case.

The stubs use -1 for UID and GID which is what syscall_windows.go was
returning prior to 3d7cb42, and these are laundered through Lookup[GU]id
to provide the same behaviour as before.

Signed-off-by: Ian Campbell ian.campbell@docker.com

@ijc
Copy link
Contributor Author

ijc commented Jul 6, 2017

This was found by moby/tool#102 / linuxkit/linuxkit#2159. Looking through the libcontainer subdir the only file I see which uses golang.org/x/sys/unix but is not using a unix or linux tag is libcontainer/utils/utils.go. I suppose that one needs to switch to golang.org/x/sys/windows.

Commit 3d7cb42 ("Move libcontainer to x/sys/unix") switched from
`syscall.Get[ug]id` to `unix.Get[ug]id` where `unix.*` is (naturally) not
available on non-Unix platforms such as Windows.

Move the offending code into a Unix specific file and provide stubs in the
_unsupported.go case.

The stubs use `-1` for UID and GID which is what syscall_windows.go was
returning prior to 3d7cb42, and these are laundered through `Lookup[GU]id`
to provide the same behaviour as before.

Signed-off-by: Ian Campbell <ian.campbell@docker.com>
@ijc
Copy link
Contributor Author

ijc commented Jul 7, 2017

CI failure is #1505, rebased to pickup #1510.

For the use of unix.WaitStatus in ExitStatus I think I might define a local interface which satisfies what ExitStatus wants to call and which unix.ExitStatus and windows.ExitStatus both meet (i.e. the Signalled, Signal and ExitStatus methods).

@ijc
Copy link
Contributor Author

ijc commented Jul 7, 2017

I think I might define a local interface which satisfies what ExitStatus wants to call

Unfortunately unix.Signal and windows.Signal have different types. Looks like I'll need to move the code around to have unix and windows versions.

Ian Campbell added 2 commits July 7, 2017 10:21
Commit 3d7cb42 ("Move libcontainer to x/sys/unix") switched from
`syscall.WaitStatus` to `unix.WaitStatus` which is not available in Windows.
Provide a version which uses x/sys/windows.

Signed-off-by: Ian Campbell <ian.campbell@docker.com>
To satisfy the Console interface.

Signed-off-by: Ian Campbell <ian.campbell@docker.com>
ijc pushed a commit to ijc/moby-tool that referenced this pull request Jul 7, 2017
To fix Windows build issues.

Signed-off-by: Ian Campbell <ian.campbell@docker.com>
@crosbymichael
Copy link
Member

@ijc overall, we don't care about runc compiling on anything but linux. This wasn't the case before but after looking how different platforms will make their own runtimes we stopped this.

Can you just add build tags in your code around this import?

@dqminh
Copy link
Contributor

dqminh commented Jul 7, 2017

@crosbymichael should we also start cleaning up the rest of non-linux code ?

@crosbymichael
Copy link
Member

@dqminh probably. I think that will also prompt us to break out a few packages that are generally useful and that could support cross platform operations.

That is what I have been doing in containerd with console and cgroups(not cross platform ;) ) packages. The user package would be good to breakout if we have a way to make it work cross platform correctly.

@dqminh
Copy link
Contributor

dqminh commented Jul 7, 2017

Yah, my plan is to merge console once we have the change for EpollConsole, and merge with containerd/cgroups after that

@crosbymichael
Copy link
Member

crosbymichael commented Jul 7, 2017

Ya, we need that review from @mrunalp ;)

@ijc
Copy link
Contributor Author

ijc commented Jul 10, 2017

@ijc overall, we don't care about runc compiling on anything but linux. This wasn't the case before but after looking how different platforms will make their own runtimes we stopped this.

Right, I wasn't sure if that extended to libcontainer since it looked like it was trying to be portable, but I guess that is actually just baggage now.

Can you just add build tags in your code around this import?

It's being pulled in through nested vendoring, I think there is one path via moby/moby/pkg/homedir/homedir.go which @justincormack is going to fix, I have a feeling there was at least one more when I looked on Friday though, which I'll go and hunt down.

@ijc
Copy link
Contributor Author

ijc commented Jul 10, 2017

@justincormack has filed moby/moby#34040 which should avoid this call chain for our use case, so closing this.

@ijc ijc closed this Jul 10, 2017
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