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.

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

Jump to bottom

Update to Go 1.13 #1312

Merged
merged 5 commits into from
Sep 5, 2019
Merged

Update to Go 1.13 #1312

merged 5 commits into from
Sep 5, 2019

Conversation

rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Sep 4, 2019 •
edited
Loading

Summary

Closes #1210

Changes

  • Update to Go 1.13
  • Fix broken dependencies
  • Move go:misspell to dev:misspell (misspell is not limited to Go), validate our documentation and JS as well

Notes for Reviewers

Everything seems to work out-of-the-box for me apart from go get -u, which fails with go get .: cannot find package
Not sure how to debug this - refs golang/go#34079
@kschiffer @bafonins no need for review

@rvolosatovs rvolosatovs added the dependencies Pull requests that update a dependency file label Sep 4, 2019
@rvolosatovs rvolosatovs added this to the September 2019 milestone Sep 4, 2019
@rvolosatovs rvolosatovs self-assigned this Sep 4, 2019
htdvisser
htdvisser reviewed Sep 4, 2019
View reviewed changes
Copy link
Contributor

@htdvisser htdvisser left a comment •
edited
Loading

Choose a reason for hiding this comment

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

For me it already breaks at step 1 of DEVELOPMENT.md#getting-started (exactly the error in #1210):

% docker run --rm -w /tti/lorawan-stack -it golang:1.13
root@6663787fb170:/tti/lorawan-stack# git clone --branch feature/go1.13 https://github.com/rvolosatovs/lorawan-stack.git .
Cloning into '.'...
remote: Enumerating objects: 243, done.
remote: Counting objects: 100% (243/243), done.
remote: Compressing objects: 100% (150/150), done.
remote: Total 88731 (delta 138), reused 152 (delta 88), pack-reused 88488
Receiving objects: 100% (88731/88731), 32.86 MiB | 19.52 MiB/s, done.
Resolving deltas: 100% (65701/65701), done.
root@6663787fb170:/tti/lorawan-stack# make init
GO111MODULE=on go install github.com/magefile/mage
go: downloading github.com/magefile/mage v1.8.1-0.20190718165527-e1fda1a0ffba
go: extracting github.com/magefile/mage v1.8.1-0.20190718165527-e1fda1a0ffba
go: finding github.com/magefile/mage v1.8.1-0.20190718165527-e1fda1a0ffba
GO111MODULE=on go run github.com/magefile/mage -compile ./mage
Error determining list of magefiles: failed to list non-mage gofiles: exit status 1
exit status 1
make: *** [.mage/mage.make:19: mage] Error 1

go.mod Outdated
@@ -114,6 +115,8 @@ require (
google.golang.org/api v0.8.0
google.golang.org/genproto v0.0.0-20190801165951-fa694d86fc64
google.golang.org/grpc v1.23.0
// Do not update this until https://github.com/heptiolabs/healthcheck/issues/23 is resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep these comments consistent (above: Do not upgrade Echo beyond v4.1.2 - see #977 .)

@htdvisser
Copy link
Contributor

That error goes away if I echo "package lorawan_stack" > doc.go

@coveralls
Copy link

coveralls commented Sep 4, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 73.536% when pulling fc92a07 on rvolosatovs:feature/go1.13 into 00c1622 on TheThingsNetwork:master.

@htdvisser
Copy link
Contributor

This seems to be related to being outside of GOPATH, if I run this in GOPATH it works without error:

% docker run --rm -w /go/src/go.thethings.network/lorawan-stack -it golang:1.13
root@35558dd4a36a:/go/src/go.thethings.network/lorawan-stack# git clone --branch feature/go1.13 https://github.com/rvolosatovs/lorawan-stack.git .
Cloning into '.'...
remote: Enumerating objects: 243, done.
remote: Counting objects: 100% (243/243), done.
remote: Compressing objects: 100% (150/150), done.
remote: Total 88731 (delta 138), reused 152 (delta 88), pack-reused 88488
Receiving objects: 100% (88731/88731), 32.86 MiB | 15.53 MiB/s, done.
Resolving deltas: 100% (65701/65701), done.
root@35558dd4a36a:/go/src/go.thethings.network/lorawan-stack# make init
GO111MODULE=on go install github.com/magefile/mage
go: downloading github.com/magefile/mage v1.8.1-0.20190718165527-e1fda1a0ffba
go: extracting github.com/magefile/mage v1.8.1-0.20190718165527-e1fda1a0ffba
go: finding github.com/magefile/mage v1.8.1-0.20190718165527-e1fda1a0ffba
GO111MODULE=on go run github.com/magefile/mage -compile ./mage
2019/09/04 15:37:10 wrote cert.pem
2019/09/04 15:37:10 wrote key.pem
Run "./mage -l" for a list of build targets

@rvolosatovs
Copy link
Contributor Author

Hmm, ok, thanks for input - I'll investigate more.
I work in the GOPATH locally indeed, which is why I probably did not encounter the error. (and which might be the cause for golang/go#34079 as well, in fact)

@rvolosatovs rvolosatovs marked this pull request as ready for review September 4, 2019 20:57
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Sep 4, 2019 •
edited
Loading

7a073e0 was caused by the call to misspell before 7c8c4c8 was applied.
Not strictly related, but nice to fix these regardless IMO

htdvisser
htdvisser reviewed Sep 5, 2019
View reviewed changes
.mage/go.go Outdated
@@ -96,7 +96,7 @@ func (Go) packageDirs() (packageDirs []string, err error) {
goPackageDirs = packageDirs
}()

dirs, err := outputGo("list", "-f", "{{.Dir}}", "./...")
dirs, err := outputGo("list", "-f", "{{.Dir}}", "./pkg/...", "./cmd/...")
Copy link
Contributor

Choose a reason for hiding this comment

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

But . and .mage (and later sdk) also contain Go packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left this as it was and did fc92a07 instead to fix the misspell target

lorawan-stack.go Outdated Show resolved Hide resolved
htdvisser
htdvisser approved these changes Sep 5, 2019
View reviewed changes
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

htdvisser
htdvisser reviewed Sep 5, 2019
View reviewed changes
.mage/dev.go
if mg.Verbose() {
fmt.Printf("Fixing common spelling mistakes in files\n")
}
return execGo("run", "github.com/client9/misspell/cmd/misspell", "-w", "-i", "mosquitto",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like how we're now explicitly listing the files and folders that need to be spellchecked. Can we perhaps do this with globbing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no obvious/simple way to do this via globbing and it would require explicitly specifying things anyway.
I tried to do it via globbing first, but then decided it's not worth the effort, especially since the files/directories we need to spellcheck are not going to change any time soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'll approve because it's just dev tooling, but we shouldn't start accepting this kind of nastiness in "real" code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any nastiness here - we need to have a clear distinction between things we can spellcheck and things we can't. For example, we should not all of a sudden start spellchecking LICENSE or go.sum, but e.g. .editorconfig or Makefile might have comments to spellcheck. We can build a bunch of unnecessary complex globbing solutions here, but there's not point in doing that as we have pretty much constant set of directories/files to spellcheck.

@htdvisser htdvisser assigned htdvisser and unassigned rvolosatovs Sep 5, 2019
@htdvisser htdvisser merged commit 7d138ea into TheThingsNetwork:master Sep 5, 2019
@rvolosatovs rvolosatovs deleted the feature/go1.13 branch September 5, 2019 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htdvisser htdvisser htdvisser approved these changes

@bafonins bafonins Awaiting requested review from bafonins

@johanstokking johanstokking Awaiting requested review from johanstokking

@kschiffer kschiffer Awaiting requested review from kschiffer

Assignees

@htdvisser htdvisser

Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
September 2019
Development

Successfully merging this pull request may close these issues.

Mage fails on Go 1.13
3 participants
@rvolosatovs @htdvisser @coveralls