This repository has been archived by the owner. It is now read-only.

Add docker-machine-driver-xhyve Formula #48002

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@zchee
Copy link
Contributor

zchee commented Jan 12, 2016

FYI, for the driver of docker-machine, So, there is no test method.

head "https://github.com/zchee/docker-machine-driver-xhyve.git"

depends_on "docker-machine"
depends_on "go" => :head

This comment has been minimized.

@DomT4

DomT4 Jan 12, 2016

Contributor

What are you trying to do here? It needs go for the head compile or it needs a head build of go to install?

This comment has been minimized.

@DomT4

DomT4 Jan 12, 2016

Contributor

Ah, the former, it looks like. Put it in a head do block with the head url then, please.

@DomT4 DomT4 added the new formula label Jan 12, 2016

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from e7122e5 to 18d30d4 Jan 12, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

@DomT4 Fixed.
Correct?

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 18d30d4 to 9a1a6ff Jan 12, 2016

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jan 12, 2016

Yeah. That's the correct way to do head/stable conditional dependencies, Thanks!

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

@DomT4 Thanks for advise :)

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 9a1a6ff to da693b0 Jan 12, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

@DomT4 Sorry, Fix HEAD build work.

PTAL.
Thanks!

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from da693b0 to 469696e Jan 12, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

@DomT4 Fixed for failed CI

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 469696e to 36147bb Jan 12, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

@DomT4 Sorry, Added depends_on :macos => :yosemite.

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 36147bb to 41f133a Jan 12, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

Fixed for failed CI.

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch 2 times, most recently from f136216 to b01a606 Jan 12, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 12, 2016

Add version for audit errer Stable: version (machine) is set to a string without a digit

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from b01a606 to c3860a0 Jan 13, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 13, 2016

@DomT4
Ready for merge.

 * Don't recommend setuid in the caveats, suggest sudo instead.

docker-machine-driver-xhyve need root owner and uid.
Caused by Hypervisor.framework and vmnet.framework.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 13, 2016

  • Don't recommend setuid in the caveats, suggest sudo instead.

You can assign a root owner without doing setuid. You can also use sudo in that case where setuid does not suffice, no?

class DockerMachineDriverXhyve < Formula
desc "Docker Machine driver for xhyve"
homepage "https://github.com/zchee/docker-machine-driver-xhyve"
url "https://github.com/zchee/docker-machine-driver-xhyve/releases/download/v0.2.1/docker-machine-driver-xhyve"

This comment has been minimized.

@xu-cheng

xu-cheng Jan 13, 2016

Contributor

We would prefer to build from source.

This comment has been minimized.

@zchee

zchee Jan 13, 2016

Contributor

@xu-cheng When to use Go vendoring (vendor), archive/v*.*.*.tar.gz is too large.
There is a case to be the timeout when downloading use curl.
I have experienced on to test this Formula.

Nevertheless, it must be the source tarball?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 13, 2016

Member

Nevertheless, it must be the source tarball?

Yes, sorry.

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 13, 2016

@MikeMcQuaid Thanks advise.

but, setuid is Ruby function? or binary command?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 13, 2016

@zchee If something needs the setuid bit set it's a better idea to just run it as a root user instead with e.g. sudo.

@johanneswuerbach

This comment has been minimized.

Copy link

johanneswuerbach commented Jan 13, 2016

@MikeMcQuaid just to chime in on that. docker-machine-xhyve-driver is never called by the user directly, but only called by https://github.com/docker/machine when using -d xhyve. Using setuid has the advantage that all docker-machine docs still work out of the box (just with a different driver) as non of them tells to user to use sudo.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 13, 2016

@johanneswuerbach That's a problem; users should know when something requires running as root and should be specifying that with e.g. sudo every time. The setuid bit (particularly on software that's installed and owned by a Homebrew user) significantly decreases the overall security of your system so we won't allow recommending it in the caveats for core formulae, sorry.

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from c3860a0 to cfdfff3 Jan 13, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 13, 2016

@MikeMcQuaid OK. I understand for Homebrew way.

I removed setuid caveats.
Correct?

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from cfdfff3 to 02123f2 Jan 13, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 15, 2016

@bfontaine Fixed.
PTAL.

end

test do
assert_match "xhyve-memory-size", shell_output("docker-machine create --driver xhyve -h")

This comment has been minimized.

@bfontaine

bfontaine Jan 15, 2016

Member

Please use the full path to the command: #{bin}/docker-machine instead of docker-machine.

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from c599d3b to 9887cc4 Jan 15, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 15, 2016

@bfontaine Sorry for keep doing :(

but, docker-machine is different Formula's binary.
So, I referenced https://github.com/Homebrew/homebrew/blob/master/share/doc/homebrew/Formula-Cookbook.md#variables-for-directory-locations.
#{HOMEBREW_PREFIX}/bin/docker-machine is correct?

FYI, that issue also docker-machine-parallels Formula.
https://github.com/Homebrew/homebrew/blob/master/Library/Formula/docker-machine-parallels.rb#L49

@bfontaine

This comment has been minimized.

Copy link
Member

bfontaine commented Jan 15, 2016

@zchee No worries! 😃

You can refer to another formula’s directories by using e.g. Formula["docker-machine"].bin. So here you can use #{Formula["docker-machine"].bin}/docker-machine.

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 9887cc4 to 4098f3a Jan 15, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 15, 2016

@bfontaine Oh, that is very useful :)
I fixed.
PTAL.

No worries! 😃

Thanks ☺️

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 17, 2016

@bfontaine
Need this?
34ff5c8

and ping.


def install
contents = Dir["{*,.git,.gitignore}"]
gopath = buildpath/"gopath"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 17, 2016

Member

Just inline these variables.

This comment has been minimized.

@zchee

zchee Jan 17, 2016

Contributor

@MikeMcQuaid Sorry, what is inline?
Correct this?

("#{buildpath}/gopath/src/github.com/zchee/docker-machine-driver-xhyve").install contents

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 17, 2016

Member

(buildpath/"gopath/src/github.com/zchee/docker-machine-driver-xhyve").install Dir["{*,.git,.gitignore}"]


cd gopath/"src/github.com/zchee/docker-machine-driver-xhyve" do
if build.head?
git_commithash = `git rev-parse --short HEAD 2>/dev/null`.delete!("\n")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 17, 2016

Member

Can just call this variable git_hash, use --quiet and .chomp

if build.head?
git_commithash = `git rev-parse --short HEAD 2>/dev/null`.delete!("\n")
end
system "go", "build", "-o", bin/"docker-machine-driver-xhyve", "-ldflags", "'-w -s'", "-ldflags", "-X 'github.com/zchee/docker-machine-driver-xhyve/xhyve.GitCommit=#{git_commithash}+Homebrew'", "./main.go"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 17, 2016

Member

What when git_commithash isn't set?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 17, 2016

Member

Also, wrap this (and all other lines e.g. the test below) at 80 characters.

This comment has been minimized.

@zchee

zchee Jan 17, 2016

Contributor

@MikeMcQuaid Correct this?

system "go", "build", "-o", bin/"docker-machine-driver-xhyve",
  "-ldflags", "'-w -s'",
  "-ldflags", "-X 'github.com/zchee/docker-machine-driver-xhyve/xhyve.GitCommit=#{git_hash}+Homebrew'",
  "./main.go"

This comment has been minimized.

@zchee

zchee Jan 17, 2016

Contributor

@MikeMcQuaid

What when git_commithash isn't set?

This variables use GitCommit=#{git_commithash}+Homebrew'"
Go compile including commit hash magic.
Is it something strange?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 17, 2016

Member

That looks good. I guess the question is: is it a problem if git_commithash is an empty string?

This comment has been minimized.

@zchee

zchee Jan 17, 2016

Contributor

@MikeMcQuaid Case of install with --HEAD flag,

(xhyve-test) Calling .GetMachineName
(xhyve-test) Calling .DriverName
(xhyve-test) Calling .GetCreateFlags
(xhyve-test) Calling .SetConfigFromFlags
Running pre-create checks...
(xhyve-test) Calling .PreCreateCheck

# This line
(xhyve-test) DBG | ===== Docker Machine xhyve Driver Version 0.2.1 (896d3f5+Homebrew) =====

(xhyve-test) DBG |
(xhyve-test) DBG | executing: /usr/local/bin/VBoxManage -v
(xhyve-test) DBG | STDOUT: 5.0.12r104815
(xhyve-test) DBG |
(xhyve-test) DBG | STDERR:
(xhyve-test) Calling .GetConfigRaw
Creating machine...
(xhyve-test) Calling .Create
(xhyve-test) DBG | local Boot2Docker ISO version:  v1.9.1
(xhyve-test) Copying /Users/zchee/.docker/machine/cache/boot2docker.iso to /Users/zchee/.docker/machine/machines/xhyve-test/boot2docker.iso...
(xhyve-test) Creating VM...
(xhyve-test) Extracting vmlinuz64 and initrd.img from boot2docker.iso...
(xhyve-test) DBG | Mounting boot2docker.iso

If without --HEAD flag,

(xhyve-test) Calling .GetMachineName
(xhyve-test) Calling .DriverName
(xhyve-test) Calling .GetCreateFlags
(xhyve-test) Calling .SetConfigFromFlags
Running pre-create checks...
(xhyve-test) Calling .PreCreateCheck

# This line
(xhyve-test) DBG | ===== Docker Machine xhyve Driver Version 0.2.1 (+Homebrew) =====

(xhyve-test) DBG |
(xhyve-test) DBG | executing: /usr/local/bin/VBoxManage -v
(xhyve-test) DBG | STDOUT: 5.0.12r104815
(xhyve-test) DBG |
(xhyve-test) DBG | STDERR:
(xhyve-test) Calling .GetConfigRaw
Creating machine...
(xhyve-test) Calling .Create
(xhyve-test) DBG | local Boot2Docker ISO version:  v1.9.1
(xhyve-test) Copying /Users/zchee/.docker/machine/cache/boot2docker.iso to /Users/zchee/.docker/machine/machines/xhyve-test/boot2docker.iso...
(xhyve-test) Creating VM...
(xhyve-test) Extracting vmlinuz64 and initrd.img from boot2docker.iso...
(xhyve-test) DBG | Mounting boot2docker.iso

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 18, 2016

Member

+Homebrew feels a bit messy to me; maybe do something like:

if build.head?
  git_hash = `git rev-parse --short HEAD --quiet`.chomp
  git_hash = " HEAD-#{git_hash}"
end
..
"xhyve.GitCommit=Homebrew#{git_commithash}"

?

This comment has been minimized.

@zchee

zchee Jan 18, 2016

Contributor

@MikeMcQuaid Hmm...
If the user builds without Homebrew, that value will be commit hash only.
e.g.

===== Docker Machine xhyve Driver Version 0.2.1 (896d3f5) =====

I think beautifully also compatible build without Homebrew is #{git_hash}+Homebrew.

If I want to parse the version for some reason, I can determine whether build with Homebrew only regexp of a backward match($).
If Homebrew HEAD-#{git_mash}, Parse of version will be complicated.

What do you think?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 18, 2016

Member

Sorry, I'm suggesting that without Homebrew HEAD it's Homebrew and with Homebrew it's Homebrew HEAD-896d3f5. How does that sound?

This comment has been minimized.

@zchee

zchee Jan 18, 2016

Contributor

@MikeMcQuaid OK. I understand :)
I fixed and pushed now.

PTAL.

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 17, 2016

@MikeMcQuaid
Once you build now, it will fail though was previously successful. Why?

Edit: Sorry, I was successful install

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 4098f3a to 1a4f413 Jan 17, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 18, 2016

@zchee Don't worry about wrapping, looks good 👍

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 18, 2016

@MikeMcQuaid Thanks :)
All completed?

@zchee zchee force-pushed the zchee:docker-machine-driver-xhyve branch from 1a4f413 to 47fa24a Jan 18, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 18, 2016

Great! 👍 from me once green.

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 18, 2016

@MikeMcQuaid Oooh! Thanks!! :)
I wait for CI result of green...

@funkymonkeymonk

This comment has been minimized.

Copy link

funkymonkeymonk commented Jan 19, 2016

Hello. Thanks for the awesome work. I installed via the script in the PR and found that because the docker-machine driver requires sudo, the subsequent VM requires sudo for any docker-machine command run against it.

monkey@host$ docker-machine env xhyve
open /Users/monkey/.docker/machine/machines/xhyve/config.json: permission denied
monkey@host$ docker ps
Could not read CA certificate "/Users/monkey/.docker/machine/machines/xhyve/ca.pem": open /Users/monkey/.docker/machine/machines/xhyve/ca.pem: permission denied

This will create a drastic departure for the workflow of users who are used to running docker commands as non-root. I'm not sure how to best support this workflow but also do things the Homebrew way.

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 19, 2016

@funkymonkeymonk Hi,

This problem is not related to this mater.
Could you repost https://github.com/zchee/docker-machine-driver-xhyve/issues?

@funkymonkeymonk

This comment has been minimized.

Copy link

funkymonkeymonk commented Jan 19, 2016

@zchee Sorry about that. Thanks.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jan 20, 2016

Merged in 72d4d93. Thank you for another contribution to Homebrew @zchee! 😺

@DomT4 DomT4 closed this in 72d4d93 Jan 20, 2016

@zchee

This comment has been minimized.

Copy link
Contributor

zchee commented Jan 20, 2016

@DomT4 Thaanks 🎉

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016

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