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.

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

merged 1 commit into from
Sep 20, 2016

Conversation

zimmski
Copy link
Member

@zimmski zimmski commented Sep 9, 2016

  • Upgrades to Go 1.7
  • Introduces test-throughout to test with the race detector and (new)
    with the address sanitizer if it is supported by the environment
  • Cleanup of some scripts

Fixes #113

@zimmski zimmski force-pushed the upgrade-to-go-1.7 branch 4 times, most recently from fd310d6 to 0f86ced Compare September 13, 2016 09:44
@@ -31,5 +35,5 @@ script:
- make install

# Test without any coverage
- make test-verbose
- make test-throughout
Copy link
Contributor

@marxriedl marxriedl Sep 14, 2016

Choose a reason for hiding this comment

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

did you mean thorough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and as discussed I will rename it to "test-full" since that makes more sense anyway.

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 -exuo pipefail

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

Choose a reason for hiding this comment

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

I will just believe you on the regex

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 build log is proof that it is working ;-)
done

@zimmski zimmski force-pushed the upgrade-to-go-1.7 branch 4 times, most recently from fb1a93a to abee3ef Compare September 19, 2016 11:51
@@ -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

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

test:
CGO_LDFLAGS="-L`llvm-config --libdir`" go test -timeout 60s -race ./...
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

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

- Upgrades to Go 1.7
- Introduces test-full to test with the race detector and (new)
  with the address sanitizer if it is supported by the environment
- Cleanup of some scripts

Fixes go-clang/gen#113 and go-clang/gen#124
@zimmski zimmski merged commit 1bf7e3c into master Sep 20, 2016
@zimmski zimmski deleted the upgrade-to-go-1.7 branch September 20, 2016 09:35
@zimmski zimmski restored the upgrade-to-go-1.7 branch September 20, 2016 10:00
@zchee zchee deleted the upgrade-to-go-1.7 branch September 30, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants