ci: update to Go 1.16.6 on CI #8324
Conversation
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
.circleci/config.yml
Outdated
| - run: | | ||
| mkdir ~/localgo && cd ~/localgo | ||
| wget https://golang.org/dl/go1.16.6.linux-amd64.tar.gz | ||
| tar xfz go1.16.6.linux-amd64.tar.gz | ||
| echo "export PATH=$(pwd)/go/bin:$PATH" >> $BASH_ENV | ||
| - run: go version |
There was a problem hiding this comment.
What's the reason why we're doing this/what happens when we don't? Is this because the machine executor doesn't have the same version of Go as the golang ones?
There was a problem hiding this comment.
Good question, this might be unnecessary code, it was carried over from #8138. Let me dig deeper.
There was a problem hiding this comment.
Yeah this is required because the sharness test is using an Ubuntu image which is on Go 1.15.2, not the Go image.
test/sharness/lib/test-lib.sh
Outdated
| @@ -9,17 +9,15 @@ | |||
| # use the ipfs tool to test against | |||
|
|
|||
| # add current directory to path, for ipfs tool. | |||
| if test "$MAKE_SKIP_PATH" != "1"; then | |||
There was a problem hiding this comment.
What changed with MAKE_SKIP_PATH?
There was a problem hiding this comment.
I'm not sure exactly why this fixes it. Presumably something somewhere else has a side efffect of setting the PATH so that it contains the correct ipfs binary for the test, which is what the sharness tests were implicitly relying on. Something changed with 1.16 such that the ipfs binary is no longer found in the PATH. I spent a few hours trying to find where, unsuccessfully. I couldn't find any rationale for MAKE_SKIP_PATH and the tests pass when I disable it, b/c PATH is setup correctly before each test run (pointing to the coverage binary).
There was a problem hiding this comment.
I'm not totally sure, but I think the idea here is that you can locally run sharness tests on your machine from the sharness test folder without causing weird issues. Could you double check those work.
It seems like removing that flag shouldn't change anything (we set it to 1 in the makefile and then check if it equals 1).
I'm not sure exactly why this fixes it.
What was failing before and with which code changes? You've also changed a couple commands here (e.g. from BIN=$(cd .. && echo `pwd`/bin) to BIN=$(cd .. && pwd)/bin ). Any chance those were the changes that fixed things and not MAKE_SKIP_PATH?
There was a problem hiding this comment.
I did figure this out finally. It was due to this:
echo "export PATH=$(pwd)/go/bin:$PATH" >> $BASH_ENV
(link)
That writes a PATH env var to BASH_ENV, and BASH_ENV is inherited by every subshell, so every time a subshell is run, Bash applies env vars in BASH_ENV which overwrite whatever PATH was inherited from the parent process, which means setting PATH in any Makefile (or any other env in BASH_ENV) is silently ignored.
(Setting PATH with BASH_ENV is what CircleCI docs recommend....)
There was a problem hiding this comment.
LGTM. @guseggert if there's a 1.16.7 then let's use that, otherwise we can just merge.
|
Per 2021-08-17 conversation: update to 1.16.7 Short term
|
| @@ -126,6 +127,12 @@ jobs: | |||
| TEST_VERBOSE: 1 | |||
| steps: | |||
| - run: sudo apt update | |||
| - run: | | |||
| mkdir ~/localgo && cd ~/localgo | |||
| wget https://golang.org/dl/go1.16.6.linux-amd64.tar.gz | |||
Carried over from #8138
This also includes a change to use a beefier build host, which improves build times a lot (we were at 2 vCPUs, now 16 vCPUs, build time for me dropped from 30min -> 15min). It also fixes a bug that was causing the -coverprofile flag to be incorrect (due to Make subtleties, spaces were not being replaced correctly here).