Skip to content

Upgrade to Go 1.7 #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 20, 2016
Merged

Upgrade to Go 1.7 #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
language: go
go:
- 1.5
- 1.7.1

addons:
apt:
Expand All @@ -13,14 +13,20 @@ addons:
- libclang1-3.4
- libclang-3.4-dev

env:
global:
- CC=clang CXX=clang++

install:
- mkdir -p /home/travis/bin
- sudo ln -s /usr/bin/clang-3.4 /home/travis/bin/clang
- sudo ln -s /usr/bin/llvm-config-3.4 /home/travis/bin/llvm-config
- sudo ldconfig

- llvm-config --version
- llvm-config --includedir
- llvm-config --libdir
- clang --version

- make install-dependencies
- make install-tools
Expand All @@ -30,5 +36,4 @@ script:
- make install

# Test without any coverage
- make test-verbose

- make test-full
15 changes: 10 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
.PHONY: all install install-dependencies install-tools test test-verbose
.PHONY: all install install-dependencies install-tools test test-full test-verbose
Copy link
Contributor

@marxriedl marxriedl Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why is there still "test-verbose"?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is and should be test-verbose. You are thinking about the rename of "test-throughout" to "test-full".

Copy link
Contributor

Choose a reason for hiding this comment

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

True, still atm they do the same. Please correct on what I'm currently obviously not seeing.

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


ROOT_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))
export ROOT_DIR
export ROOT_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))

export CC := clang
export CXX := clang++

ARGS := $(wordlist 2,$(words $(MAKECMDGOALS)),$(MAKECMDGOALS))
$(eval $(ARGS):;@:) # turn arguments into do-nothing targets
Expand All @@ -14,7 +16,10 @@ install:
install-dependencies:
go get -u github.com/stretchr/testify/...
install-tools:

test:
CGO_LDFLAGS="-L`llvm-config --libdir`" go test -timeout 60s -race ./...
CGO_LDFLAGS="-L`llvm-config --libdir`" go test -timeout 60s ./...
test-full:
$(ROOT_DIR)/scripts/test-full.sh
test-verbose:
Copy link
Contributor

Choose a reason for hiding this comment

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

test-full does as of now the same as test-verbose, since we discussed that the future command should be full, delete verbose or at least explain me why its still 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.

"test-verbose" is just "test" but with verbose output. "test-full" adds should add additional tests, like the memory sanitizer, and later maybe integration/system tests. So I think it would be wiser to just call test-verbose in test-full, and then later activate the memory sanitzer check when it is ready. I just wanted to have a single make target for the CI and I do not want to include additional checks into test/test-verbose. Now that you mentioned it, I am also voting to remove the -race argument from test/test-verbose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well now I see the "-v" flag. Ok, remove the -race argument since I presume this -verbose has another purpose.
Incorporate both changes, then you may ships it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The "test-verbose" is sometimes helpful because otherwise you do not see the output of a test. So for me it makes sense to keep it. I would however do all tests in test-full with the verbose flag, since we would run that in the CI.
Does that sound good to you? Also, does the above arguments for the assembly of "test-full" and the argument against -race in test/test-verbose sound good to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently slightly confused, what do you intend to do with -verbose now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the "-race" flag from "test" and "test-verbose".

Copy link
Contributor

Choose a reason for hiding this comment

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

"I would however do all tests in test-full with the verbose flag, since we would run that in the CI." <- if you as well do that then both will do the same and I don't see the merit of keeping them both

Copy link
Member Author

@zimmski zimmski Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Now we are both confused? ;-) I suggest the following:

  • Remove -race from test/test-verbose
  • Add test-full
    • Call test-verbose should be OK to just do -race and -v
    • Do a run with -race and -v
    • Do a run with -msan and -v
  • Call test-full in the CI

That makes the normal testing process (test/test-verbose) fast, and the CI would catch everything no matter what.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now. Yes, that sounds good.

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

CGO_LDFLAGS="-L`llvm-config --libdir`" go test -timeout 60s -race -v ./...
CGO_LDFLAGS="-L`llvm-config --libdir`" go test -timeout 60s -v ./...
12 changes: 12 additions & 0 deletions scripts/test-full.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/bin/bash

set -exuo pipefail

LLVM_VERSION=$(clang --version | grep --max-count=1 "clang version" | sed -r 's/^.*clang version ([0-9]+\.[0-9]+).+$/\1/')

# Test with the race detector
CGO_LDFLAGS="-L`llvm-config --libdir`" go test -timeout 60s -v -race ./...

# Test with the address sanitizer
# TODO there is maybe a problem within clang https://github.com/go-clang/gen/issues/123
# if [ $(echo "$LLVM_VERSION>=3.9" | bc -l) -ne 0 ] && [ $(find `llvm-config --libdir` | grep libclang_rt.san-x86_64.a | wc -l) -ne 0 ]; then CGO_LDFLAGS="-L`llvm-config --libdir` -fsanitize=memory" CGO_CPPFLAGS='-fsanitize=memory -fsanitize-memory-track-origins -fno-omit-frame-pointer' go test -timeout 60s -v -msan ./...; fi