-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Fix a few minor issues with building/running inside msysGit #10014
Conversation
tianon
commented
Jan 10, 2015
7d37161
to
d48f397
Compare
Signed-off-by: Andrew "Tianon" Page <admwiggin@gmail.com>
(temporarily hacked up the Jenkins windows builder to build my branch to demonstrate the glory) |
Sooooo legit On Friday, January 9, 2015, Tianon Gravi notifications@github.com wrote:
|
LGTM |
fmt.Printf("ERROR: couldn't resolve full path to the Docker binary (%v)", err) | ||
os.Exit(1) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what's going on here, but logic is changed. Now data from LookPath
will override DOCKER_BINARY
. Maybe that's your plan...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually killing me. We were shelling out to which
in order to ask the OS where docker
is, when that's literally what exec.LookPath
does (but also what exec.Command
itself will do, which is how we could even shell out to just which
in the first place).
I figured the point of touching dockerBinary
at all was to get a full path to the binary, so I switched the logic a bit to run LookPath
regardless such that DOCKER_BINARY
could be set to something like docker-1.4.1-dev
and have that be searched on the PATH
as well here instead of in every single exec.Command
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, got it.
LGTM |
Fix a few minor issues with building/running inside msysGit