hyperkit 0.20180403 (new formula) #25593
Conversation
Formula/hyperkit.rb
Outdated
| end | ||
|
|
||
| def caveats; <<~EOS | ||
| To enable qcow support in the block backend an OCaml OPAM development |
There was a problem hiding this comment.
This formula should be built with qcow support rather than having a caveat.
There was a problem hiding this comment.
Ok I though about it, thanks for your feedback
There was a problem hiding this comment.
do you have other feedbacks, or is it ok to merge this? @ilovezfs
Formula/hyperkit.rb
Outdated
| sha256 "382933118da3835056203d3d05923b554f36cc41a555a821516e11ccb7d16bf3" | ||
|
|
||
| head do | ||
| url "https://github.com/hishamhm/htop.git" |
Formula/hyperkit.rb
Outdated
| :tag => "v0.20180123", | ||
| :revision => "8bbe1da0166553a5b0ccff170bf9ccbd7d10753e" | ||
|
|
||
| head do |
There was a problem hiding this comment.
No head for new formulas, please
Formula/hyperkit.rb
Outdated
| depends_on "automake" => :build | ||
| end | ||
|
|
||
| option "with-qcow", "Enable support for qcow disk image format" |
There was a problem hiding this comment.
Remove option, build by default
There was a problem hiding this comment.
not sure to understand. Why would you like to enable qcow support for every setup?
There was a problem hiding this comment.
We want to ship as many formulas as possible without options, providing a full feature set for users without the need to compile themselves.
Formula/hyperkit.rb
Outdated
| system "opam", "init", "--no-setup" | ||
| opam_dir = "#{buildpath}/.brew_home/.opam" | ||
| ENV["CAML_LD_LIBRARY_PATH"] = "#{opam_dir}/system/lib/stublibs:/usr/local/lib/ocaml/stublibs" | ||
| ENV["OPAMUTF8MSGS"] = "1" |
There was a problem hiding this comment.
those values come from opam config env, they are necessary for opam to work properly.
Formula/hyperkit.rb
Outdated
| system "make" | ||
| end | ||
|
|
||
| bin.install "build/hyperkit" |
There was a problem hiding this comment.
Is there no make install procedure? No man page, no documentation?
There was a problem hiding this comment.
unfortunately not at the moment
There was a problem hiding this comment.
now include man page
Formula/hyperkit.rb
Outdated
| end | ||
|
|
||
| test do | ||
| pipe_output("#{bin}/hyperkit", "v") |
There was a problem hiding this comment.
We need a test that does something more than just output version number
There was a problem hiding this comment.
I agree with you; but I don't want either to boot a virtual machine. Any ideas?
There was a problem hiding this comment.
Maybe try some operation and confirm that it fails?
There was a problem hiding this comment.
@fxcoudert Look at the xhyve formula, the only test there is -v. Hyperkit is based on xhyve.
https://github.com/Homebrew/homebrew-core/blob/master/Formula/xhyve.rb
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Formula/hyperkit.rb
Outdated
| :revision => "8bbe1da0166553a5b0ccff170bf9ccbd7d10753e" | ||
|
|
||
| option "with-qcow", "Enable support for qcow disk image format" | ||
| if build.with? "qcow" |
There was a problem hiding this comment.
No option, make it default behaviour please.
Formula/hyperkit.rb
Outdated
| homepage "https://github.com/moby/hyperkit" | ||
| url "https://github.com/moby/hyperkit.git", | ||
| :tag => "v0.20180123", | ||
| :revision => "8bbe1da0166553a5b0ccff170bf9ccbd7d10753e" |
There was a problem hiding this comment.
Can we build from a source archive rather than git checkout?
There was a problem hiding this comment.
Looks like we could build from the source archives at https://github.com/moby/hyperkit/releases
Formula/hyperkit.rb
Outdated
| homepage "https://github.com/moby/hyperkit" | ||
| url "https://github.com/moby/hyperkit/archive/v0.20180123.tar.gz" | ||
| sha256 "382933118da3835056203d3d05923b554f36cc41a555a821516e11ccb7d16bf3" | ||
| version "v0.20180123" |
There was a problem hiding this comment.
Let version be automatically detected
Formula/hyperkit.rb
Outdated
| depends_on "libev" | ||
|
|
||
| depends_on :xcode => ["9.0", :build] | ||
| depends_on :macos => :yosemite |
There was a problem hiding this comment.
This is unnecessary, since Xcode 9.0 is only available on macOS 10.12.6 and later.
Formula/hyperkit.rb
Outdated
| end | ||
|
|
||
| test do | ||
| pipe_output("#{bin}/hyperkit", "v") |
There was a problem hiding this comment.
Maybe try some operation and confirm that it fails?
Formula/hyperkit.rb
Outdated
|
|
||
| system "opam", "config", "exec", "--", "make" | ||
|
|
||
| bin.install "build/hyperkit" |
There was a problem hiding this comment.
No doc, no other file to install?
There was a problem hiding this comment.
just added it to install procedure with c0de235
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Formula/hyperkit.rb
Outdated
| test do | ||
| assert_match(version.to_s, shell_output("#{bin}/hyperkit -v 2>&1")) | ||
| assert_match("kexec: failed to load kernel null", | ||
| shell_output("#{bin}/hyperkit -f kexec,null 2>&1", 134)) |
There was a problem hiding this comment.
This test failed with different messages depending on macos version. It can not boot a VM because test env does not have a CPU that supports EPT (x86 virtualization).
What can I test instead? Any idea?
There was a problem hiding this comment.
15:31:45 ==> /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -v 2>&1
15:31:45 ==> /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -f kexec,null 2>&1
15:31:45 sh: line 1: 21605 Abort trap: 6 /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -f kexec,null 2>&1
15:31:45 Error: hyperkit: failed
15:31:45 </kexec:\ failed\ to\ load\ kernel\ null/> expected to be =~
15:31:45 <"hv_vm_create failed\n">.
15:31:25 ==> /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -f kexec,null 2>&1
15:31:25 Error: hyperkit: failed
15:31:25 <134> expected but was
15:31:25 <1>.
There was a problem hiding this comment.
This seems to test the actual functionality of the virtualisation using hyperkit/xhyve/bhyve, which I do not see as a thing to be tested in the build of a brew package. Unless this test-case is a must (for eg. coverage or something) I would consider this test to be unnecessary.
If you want to test that the hyperkit executable is placed in the correct folder, and is part of the brew PATH, I would rather just do hyperkit -v (if that is the same as xhyve -v) just to see that the executable will run and return the correct Version values.
There was a problem hiding this comment.
Look at the xhyve formula, that is what they did.
https://github.com/Homebrew/homebrew-core/blob/master/Formula/xhyve.rb
There was a problem hiding this comment.
As an alternative you could test this instead hyperkit -f bootrom,null so it will look for a file and answer "Error opening bootrom "null": No such file or directory". Or you could just test and see that the shell exit code is not 0, instead of asserting that a string in the shell output would always be the same.
EDIT: This should always return error code 1, unless by some odd reason there is a file named null somewhere which it would pick up.
There was a problem hiding this comment.
Also, You do not have to use assert_match to match the text output of shell_output. shell_output by itself is an assertion, which checks for the expected exit value.
There was a problem hiding this comment.
Thanks for your feedback @artheus, I already made a test to evaluate if version number of hyperkit is correct.
I will give a try to hyperkit -f bootrom,null
There was a problem hiding this comment.
shell_output only check the exit code according to documentation
https://www.rubydoc.info/github/Homebrew/brew/master/Homebrew/Assertions#shell_output-instance_method
Formula/hyperkit.rb
Outdated
| @@ -39,7 +39,5 @@ def install | |||
|
|
|||
| test do | |||
| assert_match(version.to_s, shell_output("#{bin}/hyperkit -v 2>&1")) | |||
| assert_match('Error opening bootrom "null": No such file or directory', | |||
| shell_output("#{bin}/hyperkit -f bootrom,null 2>&1", 1)) | |||
There was a problem hiding this comment.
Same problem with hyperkit -f bootrom,null than:
https://github.com/Homebrew/homebrew-core/pull/25593/files/b4438325df9da8ef468077cafd4d4eeee86a4e1e#r204757986
11:17:44 ==> /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -f bootrom,null 2>&1
11:17:44 Error: hyperkit: failed
11:17:44 </Error\ opening\ bootrom\ "null":\ No\ such\ file\ or\ directory/> expected to be =~
11:17:44 <"Unable to create VM (-85377018)\nvmx_init: processor not supported by Hypervisor.framework\n">.
11:17:13 ==> /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -f bootrom,null 2>&1
11:17:13 sh: line 1: 24782 Abort trap: 6 /usr/local/Cellar/hyperkit/0.20180403/bin/hyperkit -f bootrom,null 2>&1
11:17:13 Error: hyperkit: failed
11:17:13 <1> expected but was
11:17:13 <134>.
I removed this test, and just check that installed hyperkit version number match formula version number
|
@ilovezfs @fxcoudert @artheus, if testing returned version number is ok for you, this formula may be ready for merge. |
Adds qcow support back in, should work across the board. Credit to [alepee](https://github.com/alepee) Pr for reference: [#25593](Homebrew/homebrew-core#25593)
|
add test case from @Brandon-Barajas and @markeissler tap. |
|
|
Formula/hyperkit.rb
Outdated
| def install | ||
| system "opam", "init", "--no-setup" | ||
| opam_dir = "#{buildpath}/.brew_home/.opam" | ||
| ENV["CAML_LD_LIBRARY_PATH"] = "#{opam_dir}/system/lib/stublibs:/usr/local/lib/ocaml/stublibs" |
There was a problem hiding this comment.
Don't assume /usr/local. You'll want to use:
Formula["ocaml"].opt_lib/"ocaml/stublibs"here I guess, or some variation thereof.
There was a problem hiding this comment.
thanks for your feedback, I just pushed an update
| inreplace "#{opam_dir}/compilers/4.05.0/4.05.0/4.05.0.comp", | ||
| '["./configure"', '["./configure" "-no-graph"' # Avoid X11 | ||
|
|
||
| ENV.deparallelize { system "opam", "switch", "4.05.0" } |
There was a problem hiding this comment.
Is this an upstream issue or an opam issue?
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
@fxcoudert Anything left to do here? |
|
Hello maintainers. It's been a long time since I first opened this pull request, I can understand you have a lot to do elsewhere, but it would be a great thing to take a few minutes to tackle this one 😘 |
|
@BrewTestBot test this please |
|
Thanks @alepee for the pull request, I am glad we have been able to push that one over the finish line! |
|
Merci @fxcoudert, and all of you who gave me feedbacks along the way for this first fomula 😃 |
brew install --build-from-source <formula>, where<formula>is the name of the formula you're submitting?brew audit --strict <formula>(after doingbrew install <formula>)?