-
Notifications
You must be signed in to change notification settings - Fork 681
Conversation
Would it make sense to volume-mount the checked out repository in the build container? Currently it's always building from a git-cloned repo rather than building the checked out code, which is kinda surprising. |
Yes. If only it worked under Fedora/RHEL (with SElinux enabled, volume mounts still "work", but most of the time you can't access anything under the mount point). I guess supporting both approaches is possible. E.g. the image can check whether $GOPATH/src/github.com/zettio/weave exists, and only clone if it doesn't. Would that be better? |
I see. It might be worth sticking a note in the code about that.
Yes, though it's possibly a bit too magic and it would be easy to accidentally build the wrong thing, e.g. when mistyping the destination in the volume mount. Add a flag, perhaps? |
Ok. How about forwarding any options to git-clone? That will remain the need for the explicit WEAVE_REPO and WEAVE_BRANCH environment settings. And if no options are provided, it can expect the bind mount. |
Sounds good. |
53bc27d
to
4b84cb0
Compare
Ok, I have respun the branch due to significant changes. It's ready for another look. |
I'm not sure about using the name 'zettio/weave-build' for the image here. There's no intent to upload it to dockerhub, so including the user name 'zettio' seems wrong. On the other hand, I'm not sure what the meaning of an unslashed image name is supposed to be. Probably it doesn't matter what a private non-pushed image is called, as long as it doesn't clash with something.
4b84cb0
to
cd06a29
Compare
As requested by @rade, this now supports bind-mounting your gopath into the container. And intermediate files are left around (a no-changes rebuild takes 15 seconds for me). |
@@ -1,8 +1,8 @@ | |||
#!/bin/sh | |||
set -e | |||
set -x -e |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Ok, one comment added, and one comment elaborated. |
I've made some tweak to the docs. See https://github.com/rade/weave/tree/dpw-build_in_container One problem I ran into is that |
So, that is clearly a problem. But switching the containerized build to use lxc-docker 1.5.0 also presents a problem, because in the container we only use docker as a client, and if the docker daemon on the host is using an earlier version, the client reports a version mismatch. In contrast, using the ancient 1.0.1 version of docker.io in the container is fine: It has all the client functionality we need. So build/Dockerfile should remain as it was, installing the ancient docker.io. But perhaps 'make prerequisites' should be modified to install lxc-docker 1.5.0. For that to work, the 'prerequisites' makefile target can't be a dependency of 'build'. So 'make prerequisites' ends up being superfluous to this PR (but it was kind of superfluous already). But what about the possibility that someone does 'make prerequisites' to build weave on their host, but already has an older version of docker installed, and is unpleasantly surprised when if gets upgraded? Perhaps docker should be removed from the prerequisite package list entirely, and we should change the docs to tell people to take care of it themselves. If I'd reached this point on my own, I'd simply give up on 'make prerequisites' and remove it. I don't have much use for it myself. If you think it is useful, tell me what it should do, and I'll make it do that. |
Making things into /var/tmp is a bit odd, and this works better in the context of the containerized build.
6bc5b7e
to
438e578
Compare
Need to fix markdown syntax errors. |
Done. |
Done. |
In fact, remove the mention of the exported images being in the container altogether. The images are in the host docker daemon, so it is easier to export them again than to go fishing around with 'docker cp'.
Please can you also fix the smoke tests to cope with the tarfiles having moved |
This adds a few tweaks and conveniences to support building weave in a container, and documents the process. The documentation covers how to build under a Fedora docker host, and so might satisfy the users who wanted to build under Fedora.