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

Make docker build on FreeBSD #13542

Merged
merged 1 commit into from Jul 29, 2015

Conversation

Projects
None yet
@kvasdopil
Contributor

kvasdopil commented May 28, 2015

Disabling unsupported drivers, and make some packages compile on FreeBSD

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack May 28, 2015

Contributor

@kvasdopil There are a bunch of problems with your PR which need to be fixed in order to have this in a state which would allow it to be merged after proper review:

  1. The PR can't be merged.
  2. There are too many commits for these changes, please squash them down. A single commit should be enough.
  3. The build tags for Linux only code shouldn't have !freebsd (aufs and all) as a build tag, they should have linux and !linux where required. example: // +build !exclude_graphdriver_aufs,!freebsd should be // +build !exclude_graphdriver_aufs,linux because aufs will only ever be on Linux. Some of these have already been changed for Windows.

Please update this PR to be based on the latest master.

Contributor

unclejack commented May 28, 2015

@kvasdopil There are a bunch of problems with your PR which need to be fixed in order to have this in a state which would allow it to be merged after proper review:

  1. The PR can't be merged.
  2. There are too many commits for these changes, please squash them down. A single commit should be enough.
  3. The build tags for Linux only code shouldn't have !freebsd (aufs and all) as a build tag, they should have linux and !linux where required. example: // +build !exclude_graphdriver_aufs,!freebsd should be // +build !exclude_graphdriver_aufs,linux because aufs will only ever be on Linux. Some of these have already been changed for Windows.

Please update this PR to be based on the latest master.

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil May 28, 2015

Contributor

@unclejack thanks, already doing this.

What about removing those "import lxc" from daemon.go? it this ok?

Contributor

kvasdopil commented May 28, 2015

@unclejack thanks, already doing this.

What about removing those "import lxc" from daemon.go? it this ok?

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack May 28, 2015

Contributor

@kvasdopil These changes couldn't have come at a better time because of the work to enable the building of the Docker daemon for Windows. The ongoing Windows work should have already taken care of that if I'm not mistaken and it should make things easier. If it's not taken care of on master, you should take a look at existing open PRs.

Contributor

unclejack commented May 28, 2015

@kvasdopil These changes couldn't have come at a better time because of the work to enable the building of the Docker daemon for Windows. The ongoing Windows work should have already taken care of that if I'm not mistaken and it should make things easier. If it's not taken care of on master, you should take a look at existing open PRs.

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil May 29, 2015

Contributor

@unclejack please take a look, seems like i finished

I think it's a good idea to remove extra build tags from _unix _windows _linux and _freebsd files, because go already filters them from being build on unsupported platforms, but i'd prefere to do that in separate branch.

Maybe there's a need to check that extra tags in tests?

Contributor

kvasdopil commented May 29, 2015

@unclejack please take a look, seems like i finished

I think it's a good idea to remove extra build tags from _unix _windows _linux and _freebsd files, because go already filters them from being build on unsupported platforms, but i'd prefere to do that in separate branch.

Maybe there's a need to check that extra tags in tests?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 29, 2015

Contributor

how are you compiling this? with hack/make.sh binary directly on the host?

Contributor

jessfraz commented May 29, 2015

how are you compiling this? with hack/make.sh binary directly on the host?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 29, 2015

Contributor

also can you give me an idea of what packages you have installed on the host other than go :) thanks

Contributor

jessfraz commented May 29, 2015

also can you give me an idea of what packages you have installed on the host other than go :) thanks

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil May 29, 2015

Contributor

@jfrazelle take a look at https://github.com/kvasdopil/docker/blob/freebsd-compat/FREEBSD-PORTING.md But remember that not all patches are in docker/master yet.

Contributor

kvasdopil commented May 29, 2015

@jfrazelle take a look at https://github.com/kvasdopil/docker/blob/freebsd-compat/FREEBSD-PORTING.md But remember that not all patches are in docker/master yet.

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil May 29, 2015

Contributor

@jfrazelle I mean we need last libnetwork and libcontainer for this pr to compile successfully

Contributor

kvasdopil commented May 29, 2015

@jfrazelle I mean we need last libnetwork and libcontainer for this pr to compile successfully

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 29, 2015

Contributor

ah awesome thanks :)

Contributor

jessfraz commented May 29, 2015

ah awesome thanks :)

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jun 2, 2015

Contributor

So any progress on that?

Contributor

kvasdopil commented Jun 2, 2015

So any progress on that?

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 16, 2015

Contributor

Can you please rebase? we'll merge it when it turns 💚 again

Contributor

calavera commented Jun 16, 2015

Can you please rebase? we'll merge it when it turns 💚 again

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Jun 17, 2015

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jun 17, 2015

Contributor

@calavera Done, but tests have failed for some reason. is it a race?

Contributor

kvasdopil commented Jun 17, 2015

@calavera Done, but tests have failed for some reason. is it a race?

Show outdated Hide outdated daemon/stats_freebsd.go
// convertStatsToAPITypes converts the libcontainer.Stats to the api specific
// structs. This is done to preserve API compatibility and versioning.
func convertStatsToAPITypes(ls *libcontainer.Stats) *types.Stats {
// TODO Windows. Refactor accordingly to fill in stats.

This comment has been minimized.

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

s/Windows/BSD

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

s/Windows/BSD

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 23, 2015

Contributor

Also need to update this comment to say freebsd, not windows.

@cpuguy83

cpuguy83 Jul 23, 2015

Contributor

Also need to update this comment to say freebsd, not windows.

Show outdated Hide outdated docker/daemon_unix.go
@@ -11,7 +11,7 @@ import (
"github.com/docker/docker/daemon"
"github.com/docker/docker/pkg/system"
_ "github.com/docker/docker/daemon/execdriver/lxc"
//"github.com/docker/docker/daemon/execdriver/lxc"

This comment has been minimized.

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

Shouldn't have commented code.
Does this import need to go into a file with linux build tag?

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

Shouldn't have commented code.
Does this import need to go into a file with linux build tag?

This comment has been minimized.

@kvasdopil

kvasdopil Jun 17, 2015

Contributor

I dont really get the point of that line, linux build seems to build ok without that.

@kvasdopil

kvasdopil Jun 17, 2015

Contributor

I dont really get the point of that line, linux build seems to build ok without that.

This comment has been minimized.

@tianon

tianon Jul 21, 2015

Member

It pulls in the driver code -- without it, lines like https://github.com/docker/docker/blob/06b922c63f660be431e4b678a50ae1ddb0dcf882/daemon/execdriver/lxc/init.go#L38 don't ever get executed, so it either needs to stay in here, or be moved to a Linux-specfic file in this same package.

@tianon

tianon Jul 21, 2015

Member

It pulls in the driver code -- without it, lines like https://github.com/docker/docker/blob/06b922c63f660be431e4b678a50ae1ddb0dcf882/daemon/execdriver/lxc/init.go#L38 don't ever get executed, so it either needs to stay in here, or be moved to a Linux-specfic file in this same package.

This comment has been minimized.

@tianon

tianon Jul 21, 2015

Member

This is similar to how database/sql drivers usually work, too.

@tianon

tianon Jul 21, 2015

Member

This is similar to how database/sql drivers usually work, too.

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 23, 2015

Contributor

Still need to address this. Can probably just go in daemon_linux.go ?

@cpuguy83

cpuguy83 Jul 23, 2015

Contributor

Still need to address this. Can probably just go in daemon_linux.go ?

@@ -1,4 +1,4 @@
// +build linux
// +build linux freebsd

This comment has been minimized.

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

Can we remove these build tags now?

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

Can we remove these build tags now?

This comment has been minimized.

@kvasdopil

kvasdopil Jun 17, 2015

Contributor

I'm not sure if _unix suffix works as expected, it's not mentioned in go documentation, so maybe _unix files should have the tags?

@kvasdopil

kvasdopil Jun 17, 2015

Contributor

I'm not sure if _unix suffix works as expected, it's not mentioned in go documentation, so maybe _unix files should have the tags?

This comment has been minimized.

@calavera

calavera Jun 17, 2015

Contributor

yes, _unix files need tags. We have that convention to differentiate what works on "unix-ish" systems and what works on windows.

@calavera

calavera Jun 17, 2015

Contributor

yes, _unix files need tags. We have that convention to differentiate what works on "unix-ish" systems and what works on windows.

@@ -0,0 +1,27 @@
package system

This comment has been minimized.

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

needs build tag?

@cpuguy83

cpuguy83 Jun 17, 2015

Contributor

needs build tag?

This comment has been minimized.

@kvasdopil

kvasdopil Jun 17, 2015

Contributor

_freebsd suffix works as expected, no need for build tag.

@kvasdopil

kvasdopil Jun 17, 2015

Contributor

_freebsd suffix works as expected, no need for build tag.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 17, 2015

Contributor

@kvasdopil there is an issue with running the integration tests that we're investigating, not really related with this PR.

Contributor

calavera commented Jun 17, 2015

@kvasdopil there is an issue with running the integration tests that we're investigating, not really related with this PR.

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jun 22, 2015

Contributor

@calavera so how is it going?

Contributor

kvasdopil commented Jun 22, 2015

@calavera so how is it going?

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 30, 2015

Contributor

@kvasdopil The issue in the integration tests should be fixed. But unfortunately we need to rebase this 😔

Contributor

calavera commented Jun 30, 2015

@kvasdopil The issue in the integration tests should be fixed. But unfortunately we need to rebase this 😔

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 3, 2015

Member

@kvasdopil could you rebase this? Let's get the FreeBSD work moving again 👍

Member

thaJeztah commented Jul 3, 2015

@kvasdopil could you rebase this? Let's get the FreeBSD work moving again 👍

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jul 13, 2015

Contributor

Ping @kvasdopil!

Contributor

icecrime commented Jul 13, 2015

Ping @kvasdopil!

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 13, 2015

Contributor

@icecrime @thaJeztah hey guys i'm alive, thanks for ping. Had some serious problems with free time for last couple of weeks, and feel very sorry for that. I'll update my PRs asap.

Contributor

kvasdopil commented Jul 13, 2015

@icecrime @thaJeztah hey guys i'm alive, thanks for ping. Had some serious problems with free time for last couple of weeks, and feel very sorry for that. I'll update my PRs asap.

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 21, 2015

Contributor

@icecrime @thaJeztah hello there. I'm back and i've updated the code (but having usual problems with ci ;( )

Contributor

kvasdopil commented Jul 21, 2015

@icecrime @thaJeztah hello there. I'm back and i've updated the code (but having usual problems with ci ;( )

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 24, 2015

Contributor

@calavera So what can i do about this? Other prs doesnt seem to have the same error, but i cant figure out how my modifications could trigger it.

Contributor

kvasdopil commented Jul 24, 2015

@calavera So what can i do about this? Other prs doesnt seem to have the same error, but i cant figure out how my modifications could trigger it.

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Jul 24, 2015

Contributor

@kvasdopil make cross is failing on Linux with this PR.

Created symlinks: bundles/1.8.0-dev/cross/linux/amd64/docker bundles/1.8.0-dev/cross/linux/amd64/docker-1.8.0-dev bundles/1.8.0-dev/cross/linux/amd64/docker-1.8.0-dev.md5 bundles/1.8.0-dev/cross/linux/amd64/docker-1.8.0-dev.sha256
Building: bundles/1.8.0-dev/cross/linux/386/docker-1.8.0-dev
daemon/execdriver/native/template/default_template.go:6:2: no buildable Go source files in /go/src/github.com/docker/docker/vendor/src/github.com/opencontainers/runc/libcontainer/apparmor
make: *** [cross] Error 1

I'll post more comments if anything else fails for me. Tests are running on my local box.

update: That was the only failure I've encountered.

Contributor

unclejack commented Jul 24, 2015

@kvasdopil make cross is failing on Linux with this PR.

Created symlinks: bundles/1.8.0-dev/cross/linux/amd64/docker bundles/1.8.0-dev/cross/linux/amd64/docker-1.8.0-dev bundles/1.8.0-dev/cross/linux/amd64/docker-1.8.0-dev.md5 bundles/1.8.0-dev/cross/linux/amd64/docker-1.8.0-dev.sha256
Building: bundles/1.8.0-dev/cross/linux/386/docker-1.8.0-dev
daemon/execdriver/native/template/default_template.go:6:2: no buildable Go source files in /go/src/github.com/docker/docker/vendor/src/github.com/opencontainers/runc/libcontainer/apparmor
make: *** [cross] Error 1

I'll post more comments if anything else fails for me. Tests are running on my local box.

update: That was the only failure I've encountered.

@@ -0,0 +1,5 @@
package main

This comment has been minimized.

@unclejack

unclejack Jul 24, 2015

Contributor

@kvasdopil You need a build tag here: // +build daemon (not here, but in this file, this is just where I commented)

@unclejack

unclejack Jul 24, 2015

Contributor

@kvasdopil You need a build tag here: // +build daemon (not here, but in this file, this is just where I commented)

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Jul 24, 2015

Contributor

The tests didn't fail for me locally.

Contributor

unclejack commented Jul 24, 2015

The tests didn't fail for me locally.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jul 24, 2015

Contributor

yes, flacky tests on windows are killing me.

Contributor

calavera commented Jul 24, 2015

yes, flacky tests on windows are killing me.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jul 24, 2015

Contributor

okay, let's move this forward.

LGTM

Contributor

calavera commented Jul 24, 2015

okay, let's move this forward.

LGTM

@icecrime icecrime removed the group/freebsd label Jul 24, 2015

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 27, 2015

Contributor

@calavera @unclejack @icecrime @thaJeztah so guys is there any chance to accept this pr?

Contributor

kvasdopil commented Jul 27, 2015

@calavera @unclejack @icecrime @thaJeztah so guys is there any chance to accept this pr?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jul 27, 2015

Contributor

I say let's merge it if you all are comfortable, I'll set up the CI this week ;)

Contributor

jessfraz commented Jul 27, 2015

I say let's merge it if you all are comfortable, I'll set up the CI this week ;)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 27, 2015

Member

Not really a Gopher, but would very much like to see this moving, so "lgtm" :-)

Member

thaJeztah commented Jul 27, 2015

Not really a Gopher, but would very much like to see this moving, so "lgtm" :-)

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Jul 27, 2015

Member

LGTM

Member

tianon commented Jul 27, 2015

LGTM

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jul 27, 2015

Contributor

This PR doesn't compile for me:

# github.com/docker/docker/pkg/reexec
pkg/reexec/command_freebsd.go:14: undefined: Self

You can copy what we do for windows:

// Self returns the path to the current process's binary.
// Uses os.Args[0].
func Self() string {
    return naiveSelf()
}
Contributor

calavera commented Jul 27, 2015

This PR doesn't compile for me:

# github.com/docker/docker/pkg/reexec
pkg/reexec/command_freebsd.go:14: undefined: Self

You can copy what we do for windows:

// Self returns the path to the current process's binary.
// Uses os.Args[0].
func Self() string {
    return naiveSelf()
}
@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 28, 2015

Contributor

@calavera done

Contributor

kvasdopil commented Jul 28, 2015

@calavera done

@@ -1,5 +1,3 @@
// +build linux

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

This needs either !windows or Linux FreeBSD.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

This needs either !windows or Linux FreeBSD.

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

done

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

done

Show outdated Hide outdated builder/internals_linux.go
@@ -1,5 +1,3 @@
// +build linux

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

!windows or Linux freebsd

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

!windows or Linux freebsd

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

done

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

done

Show outdated Hide outdated daemon/daemon_no_aufs.go
@@ -1,4 +1,4 @@
// +build exclude_graphdriver_aufs,linux
// +build exclude_graphdriver_aufs,linux !linux

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

This will pull in AUFS for Windows which is incorrect

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

This will pull in AUFS for Windows which is incorrect

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

didnt know aufs exist on windows, is it ok to rewrite it as // +build exclude_graphdriver_aufs freebsd ?

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

didnt know aufs exist on windows, is it ok to rewrite it as // +build exclude_graphdriver_aufs freebsd ?

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

It doesn't exist on Windows :). I -think- you need to keep the original +build exclude_graphdriver_aufs,linux, but the second part depends on your intent - should this be included in FreeBSD?

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

It doesn't exist on Windows :). I -think- you need to keep the original +build exclude_graphdriver_aufs,linux, but the second part depends on your intent - should this be included in FreeBSD?

This comment has been minimized.

@unclejack

unclejack Jul 28, 2015

Contributor

@jhowardmsft This adds a new file which makes this no_aufs source build on windows and freebsd.

@unclejack

unclejack Jul 28, 2015

Contributor

@jhowardmsft This adds a new file which makes this no_aufs source build on windows and freebsd.

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

daemon_no_aufs and daemon_aufs.go aren't currently being built on Windows with the tags as they are as the only call to migrateIfAufs is only made from daemon_unix.go which isn't compiled on Windows. Adding a !linux tag in daemon_no_aufs.go will mean daemon_no_aufs.go will be compiled on Windows which isn't necessary. I just verified that by deliberately putting compile errors in both daemon_no_aufs and daemon_aufs.go and successfully compiling the Windows daemon. I then added the tag above and sure enough, it tripped on the compile error I added to daemon_no_aufs. So I really don't believe this change is necessary here :)

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

daemon_no_aufs and daemon_aufs.go aren't currently being built on Windows with the tags as they are as the only call to migrateIfAufs is only made from daemon_unix.go which isn't compiled on Windows. Adding a !linux tag in daemon_no_aufs.go will mean daemon_no_aufs.go will be compiled on Windows which isn't necessary. I just verified that by deliberately putting compile errors in both daemon_no_aufs and daemon_aufs.go and successfully compiling the Windows daemon. I then added the tag above and sure enough, it tripped on the compile error I added to daemon_no_aufs. So I really don't believe this change is necessary here :)

@@ -0,0 +1,8 @@
package graphdriver

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Does this need a //+build FreeBSD tag? (Ignore if _FreeBSD filename is a valid go directive - it isn't for _unix)

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Does this need a //+build FreeBSD tag? (Ignore if _FreeBSD filename is a valid go directive - it isn't for _unix)

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

nope, _freebsd suffix works fine for freebsd

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

nope, _freebsd suffix works fine for freebsd

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Got it. Thanks for confirming. Ignore the comments then against _FreeBSD.go files.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Got it. Thanks for confirming. Ignore the comments then against _FreeBSD.go files.

@@ -0,0 +1,14 @@
package daemon

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Build tag?

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Build tag?

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

nope :)

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

nope :)

@@ -0,0 +1,9 @@
package daemon

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Build tag?

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Build tag?

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

no

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

no

@@ -11,7 +11,6 @@ import (
"github.com/docker/docker/daemon"
"github.com/docker/docker/pkg/system"
_ "github.com/docker/docker/daemon/execdriver/lxc"

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

It that really correct - removing the lxc driver on *nix? Not sure, but looks a little odd being in this PR.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

It that really correct - removing the lxc driver on *nix? Not sure, but looks a little odd being in this PR.

This comment has been minimized.

@unclejack

unclejack Jul 28, 2015

Contributor

It's just being moved to another separate source file.

@unclejack

unclejack Jul 28, 2015

Contributor

It's just being moved to another separate source file.

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Thanks for confirming.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Thanks for confirming.

@@ -0,0 +1,18 @@
package operatingsystem

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Needs build tag

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Needs build tag

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

still no

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

still no

@@ -0,0 +1,7 @@
package sysinfo

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Needs build tag

@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

Needs build tag

This comment has been minimized.

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

no tag needed :)

@kvasdopil

kvasdopil Jul 28, 2015

Contributor

no tag needed :)

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jul 28, 2015

Contributor

LGTM from a Windows perspective now.

Contributor

jhowardmsft commented Jul 28, 2015

LGTM from a Windows perspective now.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jul 28, 2015

Contributor

Tests pass, but CI is still very broken(unrelated to this). Code LGTM.

Contributor

calavera commented Jul 28, 2015

Tests pass, but CI is still very broken(unrelated to this). Code LGTM.

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 29, 2015

Contributor

So, what now?

Contributor

kvasdopil commented Jul 29, 2015

So, what now?

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Jul 29, 2015

Contributor

@jfrazelle You're assigned to this, can you please take a look and merge if you're OK with this PR?

Contributor

unclejack commented Jul 29, 2015

@jfrazelle You're assigned to this, can you please take a look and merge if you're OK with this PR?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jul 29, 2015

Contributor

ugh i am so sorry it needs a rebase then i promise to merge once pinged

Contributor

jessfraz commented Jul 29, 2015

ugh i am so sorry it needs a rebase then i promise to merge once pinged

@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 29, 2015

Contributor

@jfrazelle done :) ping!

Contributor

kvasdopil commented Jul 29, 2015

@jfrazelle done :) ping!

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jul 29, 2015

Contributor
---> Making bundle: validate-lint (in bundles/1.8.0-dev/validate-lint)
Errors from golint:
daemon/execdriver/execdrivers/execdrivers_freebsd.go:12:1: exported function NewDriver should have comment or be unexported
Contributor

jessfraz commented Jul 29, 2015

---> Making bundle: validate-lint (in bundles/1.8.0-dev/validate-lint)
Errors from golint:
daemon/execdriver/execdrivers/execdrivers_freebsd.go:12:1: exported function NewDriver should have comment or be unexported
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jul 29, 2015

Contributor

got a go lint error

Contributor

jessfraz commented Jul 29, 2015

got a go lint error

make docker compile on freebsd
Signed-off-by: Alexey Guskov <lexag@mail.ru>
@kvasdopil

This comment has been minimized.

Show comment
Hide comment
@kvasdopil

kvasdopil Jul 29, 2015

Contributor

@jfrazelle fixed (and i'm wondering why i haven't seen that earlier)

Contributor

kvasdopil commented Jul 29, 2015

@jfrazelle fixed (and i'm wondering why i haven't seen that earlier)

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jul 29, 2015

Contributor

test failures unrelated LGTM, merging and will get CI this week \o/

Contributor

jessfraz commented Jul 29, 2015

test failures unrelated LGTM, merging and will get CI this week \o/

jessfraz pushed a commit that referenced this pull request Jul 29, 2015

@jessfraz jessfraz merged commit 75f8bdd into moby:master Jul 29, 2015

1 of 4 checks passed

experimental Jenkins build Docker-PRs-experimental 4188 has failed
Details
janky Jenkins build Docker-PRs 12800 has failed
Details
windows Jenkins build Windows-PRs 9494 has failed
Details
docker/dco-signed All commits signed
Details
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 29, 2015

Member

Oh! Merged! Thanks @kvasdopil !!

Member

thaJeztah commented Jul 29, 2015

Oh! Merged! Thanks @kvasdopil !!

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jul 29, 2015

Contributor

🤘

Contributor

calavera commented Jul 29, 2015

🤘

@kvasdopil kvasdopil deleted the kvasdopil:freebsd-work branch Jul 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment