Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

enable 'clean', 'test' etc Makefile targets to be run in containerised build #1850

Closed
itamaro opened this issue Jan 3, 2016 · 11 comments · Fixed by #1861
Closed

enable 'clean', 'test' etc Makefile targets to be run in containerised build #1850

itamaro opened this issue Jan 3, 2016 · 11 comments · Fixed by #1861
Assignees
Labels
Milestone

Comments

@itamaro
Copy link

itamaro commented Jan 3, 2016

In Building Weave docs, it would be useful to have some info on how to run specific Makefile commands in the not-so-obvious build scenarios, where one doesn't directly run a make command.

Specifically, when looking into adding a feature to weave by building it in a container, I was able to build it, but not quite sure how to run other Makefile commands, like clean and tests.

More verbose dev-ramp-up docs can help in making it easier for occasional contributors to contribute :-)

@rade
Copy link
Member

rade commented Jan 3, 2016

This is actually more than a docs issue - building is the only thing currently supported by the weave-build container.

@itamaro
Copy link
Author

itamaro commented Jan 3, 2016

Ah, too bad. I thought the build container was just a clever way to containerize the Makefile "as is".

@rade
Copy link
Member

rade commented Jan 3, 2016

Ah, too bad. I thought the build container was just a clever way to containerize the Makefile "as is".

Almost, but not quite. See https://github.com/weaveworks/weave/blob/master/build/build.sh. It wouldn't take much effort to extend that to invoke arbitrary Makefile targets. The question is how many of them will actually work.

@itamaro
Copy link
Author

itamaro commented Jan 4, 2016

Here's a minimal hack I applied to build.sh to get it to support build, clean and tests make-targets:

diff --git a/build/build.sh b/build/build.sh
index 92db534..4215034 100644
--- a/build/build.sh
+++ b/build/build.sh
@@ -6,7 +6,11 @@ export GOPATH

 WEAVE_SRC=$GOPATH/src/github.com/weaveworks/weave

-if [ $# -eq 0 -o "$1" = "tests" ] ; then
+if [ $# -eq 0 -o "$1" = "build" -o "$1" = "tests" -o "$1" = "clean" ] ; then
+    MAKE_TARGET="$1"
+    if [ $# -eq 0 ]; then
+        MAKE_TARGET="build"
+    fi
     # No arguments.  Expect that the weave repo will be bind-mounted
     # into $GOPATH
     if ! [ -e $WEAVE_SRC ] ; then
@@ -36,7 +40,7 @@ EOF
     echo "weave:*:::::::" >>/etc/shadow
     echo "weave        ALL=(ALL)       NOPASSWD: ALL" >>/etc/sudoers

-    su weave -c "PATH=$PATH make -C $WEAVE_SRC build"
+    su weave -c "PATH=$PATH make -C $WEAVE_SRC $MAKE_TARGET"
 else
     # There are arguments to pass to git-clone
     mkdir -p ${WEAVE_SRC%/*}

with this, I can do:

# build
>docker run -v /var/run/docker.sock:/var/run/docker.sock -v ~/work/yowza/go:/home/go weaveworks/weave-build 
# tests
>docker run -v /var/run/docker.sock:/var/run/docker.sock -v ~/work/yowza/go:/home/go weaveworks/weave-build tests
# clean
>docker run -v /var/run/docker.sock:/var/run/docker.sock -v ~/work/yowza/go:/home/go weaveworks/weave-build clean

Not submitting a pull request as it's probably too hacky, but maybe useful for others.

@tomwilkie
Copy link
Contributor

I remember @dpw being against this (we used to run the tests in the build container) - see #1204.

FWIW, we do this in scope - run all the builds and tests in a container, which just runs make and whatever arguments were passed to the container - see https://github.com/weaveworks/scope/blob/master/backend/build.sh

@itamaro
Copy link
Author

itamaro commented Jan 4, 2016

What's the issue with using the build container for running tests? And clean-building? I don't see any concrete reasoning against it in the linked issues / PR's.

Not that I'd mind using a separate test-container, but I don't see one available. Nor one for clean-building.

@dpw
Copy link
Contributor

dpw commented Jan 4, 2016

I remember @dpw being against this (we used to run the tests in the build container) - see #1204.

I was against putting special cases into the build container scripts to support running tests. I'm not in general against running tests in containers - I think it's a fine idea.

(I have gained more experience with build containers, and test containers too, and how to integrate them with makefiles. I think I'm close to knowing how to do it "right". It would be nice to overhaul the weave net build stuff accordingly, although I'm not sure there is much point in doing that without general agreement to do all builds in containers - the complexity of supporting both modes is probably not worthwhile.)

@rade
Copy link
Member

rade commented Jan 4, 2016

my $0.02...

Let's not boil the ocean here. IMO the scope build script is a good model to follow, i.e. abandon support for "clone and build" and instead always require a volume mount of a $GOPATH (yes, that's a bit weird since you don't actually need go on the host), and simply pass all provided args to 'make'.

@itamaro
Copy link
Author

itamaro commented Jan 4, 2016

Let's not boil the ocean here. IMO the scope build script is a good model to follow, i.e. abandon support for "clone and build" and instead always require a volume mount of a $GOPATH (yes, that's a bit weird since you don't actually need go on the host), and simply pass all provided args to 'make'.

+1

@tomwilkie
Copy link
Contributor

+1

@tomwilkie
Copy link
Contributor

without general agreement to do all builds in containers

I believe this is also the way to go (we've gone that way with Scope too) - and becomes very important when we start vendoring, as we want to know to know that we are definitely using the vendored libraries.

@rade rade changed the title Add clean & tests to the Build docs enable 'clean', 'test' etc Makefile targets to be run in containerised build Jan 6, 2016
@tomwilkie tomwilkie self-assigned this Jan 8, 2016
@awh awh closed this as completed in #1861 Jan 13, 2016
@awh awh added this to the 1.5.0 milestone Jan 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants