Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Fix docker-machine-driver-xhyve HEAD #49467

Closed
wants to merge 3 commits into from
Closed

Fix docker-machine-driver-xhyve HEAD #49467

wants to merge 3 commits into from

Conversation

PerilousApricot
Copy link

Tested with and w/o --HEAD, and with audit.

DomT4
DomT4 reviewed Feb 24, 2016
View reviewed changes
Library/Formula/docker-machine-driver-xhyve.rb
git_hash = "HEAD-#{git_hash}"
ENV["CGO_LDFLAGS"] = "#{build_root}/vendor/build/lib9p/lib9p.a -L#{build_root}/vendor/lib9p"
ENV["CGO_CFLAGS"] = "-I#{build_root}/vendor/lib9p"
mkdir "vendor/build"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not being handled by the below make step?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib9p doesn't need the LDFLAGS, it's so the go build below will find them properly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I meant the mkdir directives rather than the flags.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. TBH, I'm not sure why the build system is the way it is. It's my first go project I've dug into the source for, and one of my first attempts at making a brew formula. The directories are in .gitignore, so they're really not supposed to be in the source tree. Why the build doesn't doesn't make them itself is a mystery to me.

What would you like for me to do in this case? Would it be better to patch the Makefile instead of the formula? I've notified the upstream of the problem, so I'm wary that such a patch would fail to apply once the issue is solved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow reply; not around Homebrew a huge amount this past week. Let's shorten it a bit:

(buildpath/"vendor/build/lib9p/backend").mkpath
(buildpath/"vendor/build/lib9p/transport").mkpath
(buildpath/"vendor/build/lib9p/sbuf").mkpath

That saves us two extra lines. If this is being addressed upstream as well it'd be nice to include a link to that PR/Issue so we know when it's safe to remove.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Ruby version of mkdir -p is just mkdir_p, and mkpath is an alias of that :). Outside of Homebrew you'd call it with FileUtils.mkdir_p for example, which may be why it didn't show up on searching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on the small cleanup? 😃

Any thoughts on this @zchee?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be best if I make the suggested cleanup and add the formula as-is? Once HEAD becomes the released version, another change will have to be made, and we can make another pass at tidying it all up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fix make job in machine-drivers/docker-machine-driver-xhyve#106
mkdir line is no longer required. It creates build directory automatically even Homebrew.
I tested local brew command.

@DomT4 WDYT?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me update the patch with your suggestions. One sec.

@PerilousApricot PerilousApricot mentioned this pull request Feb 24, 2016
Open
@PerilousApricot PerilousApricot mentioned this pull request Feb 25, 2016
Closed
@PerilousApricot
Copy link
Author

Working w/upstream @ machine-drivers/docker-machine-driver-xhyve#98

@zchee
Copy link
Contributor

zchee commented Mar 15, 2016

@PerilousApricot @DomT4 Sorry for a late reply 😖
I'll check it.

zchee
zchee reviewed Mar 15, 2016
View reviewed changes
Library/Formula/docker-machine-driver-xhyve.rb
@@ -22,13 +22,21 @@ def install
(buildpath/"gopath/src/github.com/zchee/docker-machine-driver-xhyve").install Dir["{*,.git,.gitignore}"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make lib9p job will be run git submodule update --init.
In Homebrew, will occur

==> make lib9p
git submodule update --init
fatal: no submodule mapping found in .gitmodules for path 'vendor/lib9p'
mk/lib9p.mk:36: recipe for target 'vendor/lib9p/%.c' failed
make: *** [vendor/lib9p/%.c] Error 128

So, needs fix Dir["{*,.git,.gitignore}"] -> Dir["{*,.git,.gitignore,.gitmodules}"]

@PerilousApricot
Copy link
Author

The new patch works for me.

@dunn
Copy link
Contributor

dunn commented Mar 17, 2016

Sorry for not catching this sooner, but it would be ideal to include the submodules as resources so they can be downloaded ahead of time with brew fetch and cached.

@PerilousApricot
Copy link
Author

@dunn - That's a neat feature. I don't see it listed in the cookbook, though. Mind pointing me to an example?

@dunn
Copy link
Contributor

dunn commented Mar 17, 2016

llvm.rb is a good example of resource, but I forgot that in this case the easiest thing is to have the url be the git repository rather than the tarball, since when we fetch repos we automatically fetch submodules as well. So I'd try changing

url "https://github.com/zchee/docker-machine-driver-xhyve/archive/v0.2.2.tar.gz"

to

url "https://github.com/zchee/docker-machine-driver-xhyve.git", 
    :tag => "v0.2.2",
    :revision => "7a7e30b80a9ee444e5e67fd1839422e201a1b328"

@PerilousApricot
Copy link
Author

Cool, updated with your suggestions and added a caveat

@dunn
Copy link
Contributor

dunn commented Mar 17, 2016

🙀

        Error: 1 problem in 1 formula
docker-machine-driver-xhyve:
 * 6: Trailing whitespace was found

@PerilousApricot
Copy link
Author

"Oh, let me just make one quick addition before I commit this. It's such a
small change, I won't mess it up" - always false

It's dark in this basement.

@Strech Strech mentioned this pull request Apr 29, 2016
Closed
@FiloSottile
Copy link
Sponsor Contributor

This seems good to merge, and it fixes the --HEAD build.

@ghost ghost removed the needs response label Apr 29, 2016
@dunn
Copy link
Contributor

dunn commented Apr 29, 2016

@BrewTestBot test this please

@FiloSottile
Copy link
Sponsor Contributor

@PerilousApricot I think it's just the trailing whitespace, can you clean it up please? :)

@PerilousApricot
Copy link
Author

@FiloSottile certainly. I guess I need to re-open a new PR against the brew repo?

@dunn
Copy link
Contributor

dunn commented May 2, 2016

No, we can merge this PR into homebrew/core once it passes the CI tests!

@UniqMartin
Copy link
Contributor

Merged in Homebrew/homebrew-core@691ca49 and Homebrew/homebrew-core@e1bd08c (some style adjustments). Thank you for this contribution to Homebrew, @PerilousApricot! 🎉

@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@PerilousApricot @zchee @dunn @FiloSottile @UniqMartin @DomT4