8000 Containerized weave build by dpw · Pull Request #387 · weaveworks/weave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

Containerized weave build #387

Merged
merged 13 commits into from
Feb 25, 2015
Merged

Containerized weave build #387

merged 13 commits into from
Feb 25, 2015

Conversation

dpw
Copy link
Contributor
@dpw dpw commented Feb 4, 2015

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.

@rade
Copy link
Member
rade commented Feb 6, 2015

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.

@dpw
Copy link
Contributor Author
dpw commented Feb 6, 2015

Would it make sense to volume-mount the checked out repository in the build container?

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?

@rade
Copy link
Member
rade commented Feb 6, 2015

with SElinux enabled, volume mounts still "work", but most of the time you can't access anything under the mount point

I see. It might be worth sticking a note in the code about that.

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?

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?

@dpw
Copy link
Contributor Author
dpw commented Feb 6, 2015

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?

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.

@rade
Copy link
Member
rade commented Feb 6, 2015

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.

@dpw dpw force-pushed the build_in_container branch from 53bc27d to 4b84cb0 Compare February 9, 2015 13:51
@dpw
Copy link
Contributor Author
dpw commented Feb 9, 2015

Ok, I have respun the branch due to significant changes. It's ready for another look.

dpw added 6 commits February 10, 2015 16:05
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.
@dpw dpw force-pushed the build_in_container branch from 4b84cb0 to cd06a29 Compare February 10, 2015 16:46
@dpw
Copy link
Contributor Author
dpw commented Feb 10, 2015

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.

This comment was marked as abuse.

@dpw
Copy link
Contributor Author
dpw commented Feb 19, 2015

Ok, one comment added, and one comment elaborated.

@rade
Copy link
Member
rade commented Feb 24, 2015

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 make prequisites installs the docker.io package, which replaced my lxc-docker 1.5.0 with an ancient 1.0.1. The incantations to use lxc-docker are in our Vagrantfile.

@dpw
Copy link
Contributor Author
dpw commented Feb 24, 2015

One problem I ran into is that make prequisites installs the docker.io package, which replaced my lxc-docker 1.5.0 with an ancient 1.0.1.

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.

rade and others added 2 commits February 24, 2015 14:31
Making things into /var/tmp is a bit odd, and this works better in
the context of the containerized build.
@dpw
Copy link
Contributor Author
dpw commented Feb 25, 2015

Need to fix markdown syntax errors.

@dpw
Copy link
Contributor Author
dpw commented Feb 25, 2015

Need to fix markdown syntax errors.

Done.

@dpw
Copy link
Contributor Author
dpw commented Feb 25, 2015

Need to fix markdown syntax errors.

Done.

dpw added 2 commits February 25, 2015 14:29
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'.
rade added a commit that referenced this pull request Feb 25, 2015
@rade rade merged commit d9cdfff into weaveworks:master Feb 25, 2015
@rade rade removed the in progress label Feb 25, 2015
@bboreham
Copy link
Contributor

Please can you also fix the smoke tests to cope with the tarfiles having moved

bboreham added a commit that referenced this pull request Feb 27, 2015
rade added a commit that referenced this pull request Feb 27, 2015
@rade rade modified the milestone: 0.10.0 Apr 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0