-
Notifications
You must be signed in to change notification settings - Fork 177
Improve make -f docker.Makefile vendor #482
Conversation
Initial work from @ijc brought a containerized vendoring which was great but had a few issues: - did not work on Windows: dependencies containing symlinks could not be written by golang/dep on a cifs-mounted volume (wich is the case when mounting a windows dir in Docker Desktop) - slow: 2 reasons for that: doing the vendoring accross a cifs or osxfs bind mount was far from ideal due to FS latency and caching issues. There was no cache preservation between 2 calls. This fix all those issues by: - Not using bind mounts: we resolve the vendoring directly in the container RootFS, then copy back the vendor dir and the Gopkg.lock file (which might have been updated) to the host - Mounting a volume in which golang/dep will keep repo info and git clones in cache Note that the dep-cache mount point and DEPCACHEDIR env var are //dep-cache and not /dep-cache. From a Unix FS point of view both paths are equivalent. But some windows shells (like the bash version shipped with Git on Windows) tries to do clever conversions to win32 paths when detecting unix-style paths in command line arguments, which makes docker trying to mount the volume in "c:\Program Files\Git\bin\..." which makes no sense in a Linux container. Doubling the initial "/" disable this behavior. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
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.
LGTM 🐯
The only "flaw" here is that we may end up with a container still running at the end if something fails
Yes, the exact reason why the target starts with an ugly |
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Codecov Report
@@ Coverage Diff @@
## master #482 +/- ##
=========================================
- Coverage 69.53% 69.4% -0.13%
=========================================
Files 51 50 -1
Lines 2629 2553 -76
=========================================
- Hits 1828 1772 -56
+ Misses 569 545 -24
- Partials 232 236 +4
Continue to review full report at Codecov.
|
docker.Makefile
Outdated
docker run --rm -v "$(CURDIR)":/go/src/github.com/docker/app/ $(DEV_IMAGE_NAME) make vendor | ||
rm -rf ./vendor | ||
docker rm -f docker-app-vendoring || echo "" | ||
docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor" |
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.
why the double //
?
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.
mingw flavor of sh (which is used under the hood by make) tries to be clever and when calling a non-mingw executable (not sure how it does the distinction between vanilla win32 executable and those compiled in a mingw environment though), tries to expand any argument that looks like a unix path into the equivalent windows path. docker CLI is not a mingw executable, so it expands things like /dep-cache into c:\<path to mingw root\dep-cache
. Using the double //
disable this expansion mechanism.
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 for explaining!
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.
Is the rm -rf ./vendor
needed? I see the makefile already removes;
Line 109 in 151e1c5
$(call rmdir,vendor) |
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.
@simonferquel would be good to add a comment about the double slashes.
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 think setting MSYS_NO_PATHCONV=1
disables the behavior
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.
see
moby/moby#12590 (comment) (and https://stackoverflow.com/a/34386471/1811501)
(and some other tickets https://github.com/moby/moby/search?q=MSYS_NO_PATHCONV&type=Issues)
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.
MSYS_NO_PATHCONV=1 disables this behavior with git bash. It does not seem to do it on my cygwin environment.
The only thing that seems to uniformally work (without causing problems for mounting), is the double "//".
I will comment on that
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.
the rm -rf ./vendor has to happen locally, so that we reset it with the equivalent coming from the container results (if we don't do it, we won't properly delete files that have been removed by the new vendoring)
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.
It goes further than that:
$ touch vendor/github.com/docker/cli/cli/deleteme.go
$ dep ensure -v
# Bringing vendor into sync
(1/1) Wrote github.com/docker/cli (from https://github.com/chris-crone/cli)@d6bfd7e5592dad85969516c131d33910fa5ebd58: hash of vendored tree didn't match digest in Gopkg.lock
so if the package exists in vendor/
it insists that it is exactly what it wrote there previously (rather than, say, fixing it like you probably wanted it to!). I don't see any flag which changes this behaviour.
docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor" | ||
docker start -i -a docker-app-vendoring | ||
docker cp docker-app-vendoring:/go/src/github.com/docker/app/vendor . | ||
docker cp docker-app-vendoring:/go/src/github.com/docker/app/Gopkg.lock . |
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.
Will it ever update the Gopkg.toml
? I thought sometimes it did, I might be wrong though...
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.
Not the toml no, only the lock
docker.Makefile
Outdated
docker rm -f docker-app-vendoring || echo "" | ||
docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor" | ||
docker start -i -a docker-app-vendoring | ||
docker cp docker-app-vendoring:/go/src/github.com/docker/app/vendor . | ||
docker cp docker-app-vendoring:/go/src/github.com/docker/app/Gopkg.lock . | ||
docker rm -f docker-app-vendoring | ||
git add Gopkg.lock vendor |
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.
The permissions changes on the vendored files (that I can't comment on directly so commenting here) are odd, I don't think they matter to us (we won't need to executre any of the scripts, and for one file the x
bit looks spurious since it is a .go
). My only concern is that it might be due to OS specific behaviour (i.e. Linux people might put these back), which only really matters because if the CI check (and I suppose we'll soon find out if that is a problem).
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.
The problem here comes from git rm
that is slightly different that rm -rf.
On windows, when you modify an existing file, it will keep the mode of the original one and won't change it (as ntfs don't have a mode bitfield on files). git rm delete files and stage the deletion at the same time, which has the side effect of making a subsequent modification loose the previous version mode information.
I'll try to see if there is a flag for git rm that does not stage the deletion...
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 reverted to rm -rf because I am worried at what can happen with symlinks as well.
I suggest we make a note about "you should use the make version that comes with mingw" for better script compatibility
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.
WFM if it WFYou
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.
In general I like the approach, I had a few questions/comments though...
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 seem to have a mixture of direct and in-review comments, sorry about that. Here are the ones I thought I was posting just now...
Urk, I don't know wtf is going on with how those comments are presented (I did a per-commit review in different tags, but normally that DTRT, although I may have accidentally clicked "single comment" once or twice). Anyway, I think all of my comments have been captured in some form above. |
944d7f9
to
b14e5f3
Compare
docker.Makefile
Outdated
rm -rf ./vendor | ||
docker rm -f docker-app-vendoring || true | ||
docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor" | ||
docker start -i -a docker-app-vendoring |
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.
the container doesn't have to be running to use docker cp
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.
Oh, nvm, this start
triggers the actual vendoring, right?
An alternative would be to do the vendoring in docker build
(in a build-stage). If you're using buildkit, you could even use a RUN --mount
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.
See https://github.com/moby/buildkit/blob/master/hack/update-vendor for an example
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 think we aren't quite ready (in CI at least, if not on contributors machines) to mandate buildkit.
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.
Hm, yeah. And with the legacy builder, the caching would be written to the container's filesystem (so likely slower).
Still curious why docker create
and docker start
are done separately; could just docker run
the container (and have it exit afterwards)
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.
Yes, I removed the create and collapsed everything in start (in upcoming commit)
docker.Makefile
Outdated
docker run --rm -v "$(CURDIR)":/go/src/github.com/docker/app/ $(DEV_IMAGE_NAME) make vendor | ||
rm -rf ./vendor | ||
docker rm -f docker-app-vendoring || echo "" | ||
docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor" |
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.
Is the rm -rf ./vendor
needed? I see the makefile already removes;
Line 109 in 151e1c5
$(call rmdir,vendor) |
docker.Makefile
Outdated
@@ -110,9 +110,19 @@ lint: ## run linter(s) | |||
|
|||
vendor: build_dev_image | |||
$(info Update Vendoring...) | |||
docker run --rm -v "$(CURDIR)":/go/src/github.com/docker/app/ $(DEV_IMAGE_NAME) make vendor | |||
rm -rf ./vendor |
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.
Should this be put after the container completes vendoring? (i.e. in case the container fails, your local vendor
is gone)
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.
👍
docker.Makefile
Outdated
rm -rf ./vendor | ||
docker rm -f docker-app-vendoring || true | ||
docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor" | ||
docker start -i -a docker-app-vendoring |
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.
Hm, yeah. And with the legacy builder, the caching would be written to the container's filesystem (so likely slower).
Still curious why docker create
and docker start
are done separately; could just docker run
the container (and have it exit afterwards)
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@ijc @thaJeztah, just updated the PR. PTAL. |
- What I did
Initial work from @ijc brought a containerized vendoring which was great
but had a few issues:
written by golang/dep on a cifs-mounted volume (wich is the case when
mounting a windows dir in Docker Desktop)
bind mount was far from ideal due to FS latency and caching issues.
There was no cache preservation between 2 calls.
This PR fixes those issues.
- How I did it
container RootFS, then copy back the vendor dir and the Gopkg.lock file
(which might have been updated) to the host
clones in cache
Note that the dep-cache mount point and DEPCACHEDIR env var are
//dep-cache and not /dep-cache. From a Unix FS point of view both paths
are equivalent. But some windows shells (like the bash version shipped
with Git on Windows) tries to do clever conversions to win32 paths when detecting unix-style
paths in command line arguments, which makes docker trying to mount the
volume in "c:\Program Files\Git\bin..." which makes no sense in a Linux
container. Doubling the initial "/" disable this behavior.
- How to verify it
try
make -f docker.Makefile
on Windows