Skip to content
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

ci: add go 1.15 to tests #1621

Merged
merged 1 commit into from Oct 2, 2020
Merged

ci: add go 1.15 to tests #1621

merged 1 commit into from Oct 2, 2020

Conversation

jakesylvestre
Copy link
Collaborator

Note: this requires #1619 before it can be merged.

@jakesylvestre
Copy link
Collaborator Author

One more thing on this. Someone with permissions (likely @Roasbeef) will have to add this as a required test once we get this merged

@@ -6,7 +6,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
go: [1.13, 1.14]
go: [1.13, 1.14, 1.15]
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we should be testing on three versions. The policy has always been current and current-1. So if 1.15 is current, please drop 1.13.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jcvernaleo should be good to go here

@jakesylvestre
Copy link
Collaborator Author

jakesylvestre commented Sep 4, 2020

looks like we'll need to remove 1.13 as a required check (and switch it to 1.15)

@ipriver
Copy link
Contributor

ipriver commented Sep 5, 2020

JFYI
How about to also update README.md requirements version? If CI is testing only the 2 latest golang versions then there's no guarantee that the project works correctly with go version < 1.14.

@jakesylvestre
Copy link
Collaborator Author

@ipriver good call. I missed this

@jakesylvestre
Copy link
Collaborator Author

anyone know if this is still true?
image

Copy link
Collaborator

@onyb onyb left a comment

Choose a reason for hiding this comment

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

Assuming the next release will be built with Go 1.15, we need to remove darwin-386 from release/release.sh. See why.

@@ -14,4 +14,4 @@ require (
golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37
)

go 1.12
go 1.14
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not 1.15?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think 1.14 is fine here as it's the minimal supported version.

https://golang.org/ref/mod#go-mod-file-go
A go directive sets the expected language version for the module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. 👍

@onyb
Copy link
Collaborator

onyb commented Sep 8, 2020

@jakesyl Regarding reproducible builds, the release script is still valid for Go 1.14 / 1.15 (i.e. we still need the -trimpath build flag):

➜  btcd git:(master) ✗ go build -ldflags="-s -w -buildid=" github.com/btcsuite/btcd          
➜  btcd git:(master) ✗ go tool objdump ./btcd | grep "/Users/" | wc -l                          
    2857
➜  btcd git:(master) ✗ go build -trimpath -ldflags="-s -w -buildid=" github.com/btcsuite/btcd 
➜  btcd git:(master) ✗ go tool objdump ./btcd | grep "/Users/" | wc -l                       
       0

Apart from the -trimpath, there is no ugly workaround in the release script, we should be able to remove these lines:

However, this wasn't _fully_ solved in `go1.13`, as the build system still includes the
directory the binary is built into the binary itself. As a result, our scripts
utilize a work around needed until `go1.13.2`.

@onyb onyb added this to In progress in btcd-dev Sep 9, 2020
@onyb onyb added the build label Sep 9, 2020
@onyb onyb added this to the 0.22.0 milestone Sep 9, 2020
@jakesylvestre
Copy link
Collaborator Author

@onyb all set

@jakesylvestre
Copy link
Collaborator Author

ah, sorry. Need to fix release/release.sh

@jakesylvestre
Copy link
Collaborator Author

now I should be all set

@coveralls
Copy link

coveralls commented Sep 17, 2020

Pull Request Test Coverage Report for Build 258780159

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.7%) to 53.563%

Files with Coverage Reduction New Missed Lines %
database/ffldb/blockio.go 4 92.62%
peer/peer.go 6 75.87%
Totals Coverage Status
Change from base Build 253997810: 0.7%
Covered Lines: 20661
Relevant Lines: 38573

💛 - Coveralls

@jakesylvestre
Copy link
Collaborator Author

that's new

@onyb onyb moved this from In progress / Awaiting changes to Pending reviews in btcd-dev Sep 21, 2020
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

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

OK

btcd-dev automation moved this from Pending reviews to Reviewer approved Sep 21, 2020
@Roasbeef Roasbeef merged commit 40ae935 into btcsuite:master Oct 2, 2020
btcd-dev automation moved this from Reviewer approved to Done Oct 2, 2020
@jakesylvestre jakesylvestre deleted the go1.15 branch October 2, 2020 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
btcd-dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants