docker-machine-driver-xhyve: fix libev issue #6875

zchee · 2016-11-13T11:42:57Z

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Change to without make build.
libev static library file was hardcoded my Makefile, It is not a good idea to update the version for fix it.
So, I wrote the doing with the Makefile.

Close #6678

zchee · 2016-11-13T11:43:29Z

/cc @MikeMcQuaid

MikeMcQuaid · 2016-11-13T13:16:15Z

So, I wrote the doing without the Makefile.

Could the Makefile be patched instead? Then formula is pretty messy after this. Ideally if you are going to or have fixed this upstream already we'll just apply the upstream patch.

zchee · 2016-11-14T07:03:49Z

@MikeMcQuaid I see. will do.

BTW, What command is can be tested efficiently? brew audit is only lint(IIRC), What kind of command is good for install test in the sandbox?
I saw man brew, but install command haven't --dry-run flag.
I build on my repository directory with make, always use the latest dev(because I'm an owner).
So I do not want to install by brew

MikeMcQuaid · 2016-11-14T08:06:39Z

@zchee You could use brew test-bot in CI to test installation is working correctly. I'm not quite sure what you're asking, though, could you rephase/elaborate? Thanks!

zchee · 2016-11-17T02:00:14Z

@MikeMcQuaid Fixed with patch :DATA. PTAL.

Edit: In make on brew, I found it does not handle -X=GitCommit build flag, also fixed.

zchee · 2016-11-17T02:00:39Z

@MikeMcQuaid

You could use brew test-bot in CI to test installation is working correctly. I'm not quite sure what you're asking, though, could you rephase/elaborate? Thanks!

Thanks advice :)

MikeMcQuaid · 2016-11-17T15:45:54Z

Formula/docker-machine-driver-xhyve.rb

@@ -38,8 +40,12 @@ def install

      if build.with? "qcow2"
        build_tags << " qcow2"
+        ENV["LIBEV_FILE"] = "#{Formula["libev"].lib}/libev.a"


If you're using the static library you can do depends_on "libev" => :build above.

Got it. will do.

Wait for the above reply, and fix this together.

MikeMcQuaid · 2016-11-17T15:46:10Z

Formula/docker-machine-driver-xhyve.rb

@@ -23,6 +23,8 @@ class DockerMachineDriverXhyve < Formula
    depends_on "libev"
  end

+  patch :DATA


Can you please add a link to the upstream patch commit and explanation of why the patch is needed in a comment in the formula file. Thanks!

That's patch is not merge to upstream yet. Because $(LIBEV_FILE) is just carried from
https://github.com/docker/hyperkit/blob/master/Makefile#L83
I would like to follow hyperkit as much as possible. but, Now I'm considering allowing the user to specify the path. So, there is no commit url yet.

Also, sometimes Go users set $GO_LDFLAGS to shell global. i.e. always strip symbol table & DWARF with -s -w.
In this case, if change the += to ?=, GNU Make handling shell $GO_LDFLAGS env(IIRC).
It means, ignores -X=xhyve.GitCommit=(...) trick.
There is a possibility that I can not take a commit hash on the issue, So I am not going to change upstream.

Well, If some comment is necessary, I would appreciate it if I could get your advice, because I am not good at English...

Is this going to go upstream in some form? If so, when? If it's definitely going to be included in the next release I'm fine with that. How about # Allow specifying version and libev location

Ah, In other words, I will not this patch to my repository, because have side effects.
This patch is Homebrew specific.

I'll add # Allow specifying version and libev location.

Close Homebrew#6678

MikeMcQuaid · 2016-11-18T13:03:59Z

Ah, In other words, I will not this patch to my repository, because have side effects.
This patch is Homebrew specific.

I'll add # Allow specifying version and libev location.

Just to pull this out so it's visible: Homebrew does not accept Homebrew-specific patches that will never be merged upstream. We'll need to figure out something that can work both for your upstream project and Homebrew.

zchee · 2016-11-18T13:06:48Z

@MikeMcQuaid Hmm...
Do you also not admit inreplace?

MikeMcQuaid · 2016-11-18T14:29:31Z

Do you also not admit inreplace?

@zchee We'd pretty strongly rather avoid it. It makes it much harder to keep the formula up to date. What's wrong with your patch being merged upstream as-is?

zchee · 2016-11-18T15:12:45Z

@MikeMcQuaid

What's wrong with your patch being merged upstream as-is?

As I said on #6875 (comment), below are why I do not merge patches (after the __END__ in docker-machine-driver-xhyve.rb) to my repository.

$(LIBEV_FILE) is just carried from
https://github.com/docker/hyperkit/blob/master/Makefile#L83
I would like to follow hyperkit as much as possible.

Also, sometimes Go users set $GO_LDFLAGS to shell global. i.e. always strip symbol table & DWARF with -s -w.
In this case, if change the += to ?=, GNU Make handling shell $GO_LDFLAGS env(IIRC).
It means, ignores -X=xhyve.GitCommit=(...) trick.
There is a possibility that I can not take a commit hash on the issue, So I am not going to change upstream.

In other words, without brew, If some users export GO_LDFLAGS='-s -w' and make, this variable will not change to commit hash. always HEAD.
https://github.com/zchee/docker-machine-driver-xhyve/blob/master/xhyve/version.go#L8
So, I will not be able to determine the binary's commit level version.

This patch have problems without brew.
The brew command is executed within the sandbox and overwrite $GO_LDFLAGS, so there is no problem. but it is not good for self-build users.

If following the tool manager's policy, and change my repository's Makefile just for brew, it may be difficult to debug an issue.
I can not consent to it.

MikeMcQuaid · 2016-11-18T15:17:32Z

In other words, without brew, If some users export GO_LDFLAGS='-s -w' and make, this variable will not change to commit hash. always HEAD.

You could instead provide a PACKAGER variable or similar which adds to the version rather than overriding the GO_LDFLAGS as a whole.

If following the tool manager's policy, and change my repository's Makefile just for brew, it may be difficult to debug an issue.

Providing options for packagers is pretty well established as a practice.

$(LIBEV_FILE) is just carried from
https://github.com/docker/hyperkit/blob/master/Makefile#L83
I would like to follow hyperkit as much as possible.

Allowing to to be overridden seems acceptable and I suspect upstream hyperkit would accept that too. If it isn't, you could remove the qcow2 option entirely to remove one patch from here.

MikeMcQuaid · 2016-12-02T09:25:49Z

@zchee What's the latest on this?

zchee · 2016-12-02T09:29:14Z

@MikeMcQuaid I'll release v0.3.1 soon, will fix together.

MikeMcQuaid · 2016-12-02T09:40:17Z

@zchee Great, thanks.

zchee force-pushed the docker-machine-driver-xhyve_fix_ev branch from dabd83f to 8cde84a Compare November 17, 2016 01:58

MikeMcQuaid requested changes Nov 17, 2016

View reviewed changes

docker-machine-driver-xhyve: fix libev issue

ebd0469

Close Homebrew#6678

zchee force-pushed the docker-machine-driver-xhyve_fix_ev branch from 8cde84a to ebd0469 Compare November 18, 2016 12:59

zchee closed this Dec 1, 2016

tnir mentioned this pull request Mar 7, 2018

Add docker-machine-driver-hyperkit v0.24.1 Homebrew/homebrew-cask#43035

Closed

9 tasks

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker-machine-driver-xhyve: fix libev issue #6875

docker-machine-driver-xhyve: fix libev issue #6875

zchee commented Nov 13, 2016

zchee commented Nov 13, 2016

MikeMcQuaid commented Nov 13, 2016

zchee commented Nov 14, 2016

MikeMcQuaid commented Nov 14, 2016

zchee commented Nov 17, 2016 •

edited

Loading

zchee commented Nov 17, 2016

MikeMcQuaid Nov 17, 2016

zchee Nov 18, 2016

zchee Nov 18, 2016

MikeMcQuaid Nov 17, 2016

zchee Nov 18, 2016

MikeMcQuaid Nov 18, 2016

zchee Nov 18, 2016

MikeMcQuaid commented Nov 18, 2016

zchee commented Nov 18, 2016

MikeMcQuaid commented Nov 18, 2016

zchee commented Nov 18, 2016

MikeMcQuaid commented Nov 18, 2016

MikeMcQuaid commented Dec 2, 2016

zchee commented Dec 2, 2016

MikeMcQuaid commented Dec 2, 2016

docker-machine-driver-xhyve: fix libev issue #6875

docker-machine-driver-xhyve: fix libev issue #6875

Conversation

zchee commented Nov 13, 2016

zchee commented Nov 13, 2016

MikeMcQuaid commented Nov 13, 2016

zchee commented Nov 14, 2016

MikeMcQuaid commented Nov 14, 2016

zchee commented Nov 17, 2016 • edited Loading

zchee commented Nov 17, 2016

MikeMcQuaid Nov 17, 2016

Choose a reason for hiding this comment

zchee Nov 18, 2016

Choose a reason for hiding this comment

zchee Nov 18, 2016

Choose a reason for hiding this comment

MikeMcQuaid Nov 17, 2016

Choose a reason for hiding this comment

zchee Nov 18, 2016

Choose a reason for hiding this comment

MikeMcQuaid Nov 18, 2016

Choose a reason for hiding this comment

zchee Nov 18, 2016

Choose a reason for hiding this comment

MikeMcQuaid commented Nov 18, 2016

zchee commented Nov 18, 2016

MikeMcQuaid commented Nov 18, 2016

zchee commented Nov 18, 2016

MikeMcQuaid commented Nov 18, 2016

MikeMcQuaid commented Dec 2, 2016

zchee commented Dec 2, 2016

MikeMcQuaid commented Dec 2, 2016

zchee commented Nov 17, 2016 •

edited

Loading