Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

initial makefile #7

Merged
merged 4 commits into from
Apr 19, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
cri-containerd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we compile the binary into a output directory? Either bin or _output.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

20 changes: 20 additions & 0 deletions .tool/lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand, why do we need to put this in .tool, but put verify-gofmt in hack/?
Why can't we all move them into hack? Is there any problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason was following the cri-o pattern

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure will move and explain these "_" will be removed when used/implemented

Copy link
Member

@Random-Liu Random-Liu Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether the cri-o pattern is intended. To me, it makes more sense to move them all into hack/ if there is no particular reason why we should separate them.


set -o errexit
set -o nounset
set -o pipefail

for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -not -iwholename '*vendor*'); do
Copy link
Member

@Random-Liu Random-Liu Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use not? If we only care about go files, I believe most of our go files will be in cmd and pkg.

How about find . -type d -a \( -iwholename './pkg*' -o -iwholename './cmd*' \)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason just following the pattern from cri-o yours works just as well but would need to be updated if we add any code somewhere else...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theirs also need to be updated when we include new non-go directories, right?

The only golang package I could image we'll add is test directory. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have druthers here.. will try yours out :-)

gometalinter \
--exclude='error return value not checked.*(Close|Log|Print).*\(errcheck\)$' \
--exclude='.*_test\.go:.*error return value not checked.*\(errcheck\)$' \
--exclude='duplicate of.*_test.go.*\(dupl\)$' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. For my education, what kind of error are we trying to exclude here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/mibk/dupl
gometalinter uses a tool called dupl ^
This filters the _test.go results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the document it's not clear to me what the tool is doing, and why we should exclude the failure here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking for possible duplicated code. Typically unit test cases have some duplicated code. We could remove it and see what it does when we add unit tests :-)

--exclude='vendor\/.*' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of error are we trying to exclude here? Just for my education.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to skip lint on vendor code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, we should have skip vendor in while, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really really don't want vendor. But you're correct it probably does not need to be on both the loop filter and the --exclude filter since we are passing single files. I imagine a possibility for one of the lint tools to pull in an import and test it as well but that's a guess. I'd have to remove it to see for sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw you removed this. So we don't need this?

--disable=aligncheck \
--disable=gotype \
--disable=gas \
--cyclo-over=60 \
--dupl-threshold=100 \
--tests \
--deadline=30s "${d}"
done
84 changes: 84 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
GO ?= go
EPOCH_TEST_COMMIT ?= f2925f58acc259c4b894353f5fc404bdeb40028e
PROJECT := github.com/kubernetes-incubator/cri-containerd
GIT_BRANCH := $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null)
GIT_BRANCH_CLEAN := $(shell echo $(GIT_BRANCH) | sed -e "s/[^[:alnum:]]/-/g")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch is not used. Let's remove it and add it back when we add the version support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

PREFIX ?= ${DESTDIR}/usr/local
BINDIR ?= ${PREFIX}/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, can we use BINDIR directly?

The name PREFIX is confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose. We can always add back in later.


all: binaries

default: help

help:
@echo "Usage: make <target>"
@echo
@echo " * 'install' - Install binaries to system locations"
@echo " * 'binaries' - Build cri-containerd"
@echo " * 'clean' - Clean artifacts"
@echo " * 'lint' - Execute the source code linter"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace this with a verify command to do all the verification all together?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@echo " * 'gofmt' - Verify the source code gofmt"

.PHONY: check-gopath

check-gopath:
ifndef GOPATH
$(error GOPATH is not set)
endif

lint: check-gopath
@echo "checking lint"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only echo information for lint?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason will modify

@./.tool/lint

gofmt:
@./hack/verify-gofmt.sh

cri-containerd: check-gopath
$(GO) build -o $@ \
$(PROJECT)/cmd/cri-containerd

clean:
find . -name \*~ -delete
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my education, what file do we want to cleanup with these 2 commands?

Copy link
Member Author

@mikebrow mikebrow Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just temp files (from md2man) that we don't have atm so can be removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

find . -name \#\* -delete
rm -f cri-containerd

binaries: cri-containerd

install: check-gopath
install -D -m 755 cri-containerd $(BINDIR)/cri-containerd

uninstall:
rm -f $(BINDIR)/cri-containerd

.PHONY: .gitvalidation
# When this is running in travis, it will only check the travis commit range
.gitvalidation: check-gopath
ifeq ($(TRAVIS),true)
git-validation -q -run DCO,short-subject
else
git-validation -v -run DCO,short-subject -range $(EPOCH_TEST_COMMIT)..HEAD
endif

.PHONY: install.tools
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why install.tools is not in help?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not PHONY following commands .install.gitvalidation etc. ?

Copy link
Member Author

@mikebrow mikebrow Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason.. usually one should phony the commands where the target is not the output of the command. I picked this up from cri-o will update.


install.tools: .install.gitvalidation .install.gometalinter .install.md2man

.install.gitvalidation:
go get -u github.com/vbatts/git-validation

.install.gometalinter:
go get -u github.com/alecthomas/gometalinter
gometalinter --install

.install.md2man:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this later when we need it. We can file an issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk

go get -u github.com/cpuguy83/go-md2man

.PHONY: \
binaries \
clean \
default \
gofmt \
help \
install \
lint \
uninstall
5 changes: 5 additions & 0 deletions cmd/cri-containerd/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type CRIContainerdOptions struct {
ContainerdConnectionTimeout time.Duration
}

// NewCRIContainerdOptions returns a reference to CRIContainerdOptions
func NewCRIContainerdOptions() *CRIContainerdOptions {
return &CRIContainerdOptions{}
}
Expand All @@ -47,6 +48,10 @@ func (c *CRIContainerdOptions) AddFlags(fs *pflag.FlagSet) {
2*time.Minute, "Connection timeout for containerd client.")
}

// InitFlags must be called after adding all cli options flags are defined and
// before flags are accessed by the program. Ths fuction adds flag.CommandLine
// (the default set of command-line flags, parsed from os.Args) and then calls
// pflag.Parse().
func InitFlags() {
pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
pflag.Parse()
Expand Down
21 changes: 21 additions & 0 deletions hack/verify-gofmt.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we use /bin/bash here, but use /usr/bin/env bash above?

Based on http://stackoverflow.com/questions/16365130/the-difference-between-usr-bin-env-bash-and-usr-bin-bash, there are pros and cons. We should at least make it consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

picked up from cri-o... don't have any druthers here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's make them consistent. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hack/* and normalize to #!/bin/bash

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done..


set -o errexit
set -o nounset
set -o pipefail

find_files() {
find . -not \( \
\( \
-wholename '*/vendor/*' \
\) -prune \
\) -name '*.go'
}

GOFMT="gofmt -s"
bad_files=$(find_files | xargs $GOFMT -l)
if [[ -n "${bad_files}" ]]; then
echo "!!! '$GOFMT' needs to be run on the following files: "
echo "${bad_files}"
exit 1
fi
3 changes: 2 additions & 1 deletion pkg/server/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

"k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"

_ "github.com/containerd/containerd/api/services/content"
_ "github.com/containerd/containerd/api/services/content" // TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing context for TODO

Copy link
Member Author

@mikebrow mikebrow Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context was described below.. none of the functions are implemented :-) "_" is being used to hold the reference from autocomplete deleting it until the reference is used below..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOW "github.com/containerd/containerd/api/services/content" is the ToDo :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the TODO above these imports with some context?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

_ "github.com/containerd/containerd/api/services/execution"
_ "github.com/containerd/containerd/api/services/images"
_ "github.com/containerd/containerd/api/services/rootfs"
Expand All @@ -41,6 +41,7 @@ type CRIContainerdService interface {
// criContainerdService implements CRIContainerdService.
type criContainerdService struct{}

// NewCRIContainerdService returns a new instance of CRIContainerdService
func NewCRIContainerdService(conn *grpc.ClientConn) CRIContainerdService {
// TODO: Initialize different containerd clients.
return &criContainerdService{}
Expand Down