gha: add golangci-lint job, fix warnings; bump API version #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolyshkin
changed the title
|
OK, this can't be done until we have a mix of .c and .go files. Trying to fix... |
This is to fix a warning from golangci-lint. As we do not have a way to return the error here, deliberately ignore it. Signed-off-by: Kir Kolyshkin <[email protected]>
1. Use log instead of fmt.Printf(). 2. Use log.Fatal* that does os.Exit(1) for us. 3. Always print the err on error. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
golangci
branch
4 times, most recently
from
7f3f204
to
48407e2
Compare
1. Remove the 'install.tools' target. In different version of Golang, "go get" has different meaning. In particular, since go 1.13 or so, go get in a package directory adds the new package to go.mod/go.sum. We don't want do add golint into magic-gen deps, so let's remove it, as well as the machinery to find out GOBIN. 2. Remove the 'lint' target. Linting will be done from the main Makefile in the next commit. Signed-off-by: Kir Kolyshkin <[email protected]>
Many Go source code tools, including linters, do not like when the sources for multiple binaries reside in the same directory. They don't like it when the unrelated .c files are found, too. Reorganise the source tree to follow the "one binary -- one directory" rule, which allows us to use golangci-lint. Now, 1. Enable golangci-lint from Makefile. 2. Improve piggie handling in Makefile. In particular: - do not rely on `pidof piggie`, get it from the run; - always kill piggie instance(s) at the end. NOTE that we can't use $PID for kill because after restore the PID is different. 3. Remove the 'install.tools' target. In different version of Golang, "go get" has different meaning. In particular, since go 1.13 or so, go get in a package directory adds the new package to go.mod/go.sum. We don't want do add golint into magic-gen deps, so let's remove it, as well as the machinery to find out GOBIN. 4. Add removing the image dir after successful test. 5. Fix the phony targets list. 6. Remove @ from some commands, as we want to see what's happening. Signed-off-by: Kir Kolyshkin <[email protected]>
...and add a badge to README. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
golangci
branch
2 times, most recently
from
84d2799
to
ffc8aa6
Compare
kolyshkin
changed the title
This fixes warnings like this one: > main.go:12:2: SA1019: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (staticcheck) > "github.com/golang/protobuf/proto" > ^ (and there's no functional or API change as far as I can see). Note that github.org/google/protobuf is still there in go.mod as it is used by the .pb.go files. Apparently it is there only to ensure backward compatibility with something like: > import proto "github.com/golang/protobuf/proto" > // This is a compile-time assertion that a sufficiently up-to-date version > // of the legacy proto package is being used. > const _ = proto.ProtoPackageIsVersion4 Signed-off-by: Kir Kolyshkin <[email protected]>
rpc.CriuOpts contains a mutex and therefore should not ever be passed by
value (the other reason is it probably uses too much stack space --
though I haven't checked that).
This fixes a bunch of warnings from govet and copylocks linters, like:
> main.go:190:26: copylocks: Dump passes lock by value: github.com/checkpoint-restore/go-criu/v4/rpc.CriuOpts contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
> func (c *Criu) Dump(opts rpc.CriuOpts, nfy Notify) error {
> ^
> main.go:195:29: copylocks: Restore passes lock by value: github.com/checkpoint-restore/go-criu/v4/rpc.CriuOpts contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
> func (c *Criu) Restore(opts rpc.CriuOpts, nfy Notify) error {
> ^
Fix the prototypes, and adjust all the callers accordingly.
Finally, as this changes the public API, bump the version to v5.
Signed-off-by: Kir Kolyshkin <[email protected]>
The meaning of it is to make sure `go mod tidy` was run after any module-related changes. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
changed the title
|
@adrianreber @rst0git PTAL. Unfortunately I had to change the public API and thus bump v4 to v5 -- I see no way around it. |
Closed
|
Thanks. Looks good to me. I have no problem doing a v5 based release. @rst0git what do you think about these changes? |
rst0git
reviewed
Feb 16, 2021
Brought to you by gofumpt v0.1.0. Signed-off-by: Kir Kolyshkin <[email protected]>
> main.go:48:9: G204: Subprocess launched with function call as argument or cmd arguments (gosec) > cmd := exec.Command(c.swrkPath, args...) > ^ This warning is targeted more towards the code audit. Signed-off-by: Kir Kolyshkin <[email protected]>
> test/main.go:27:2: G109: Potential Integer overflow made by strconv.Atoi result conversion to int16/32 (gosec) > pid, _ := strconv.Atoi(pidS) > ^ Add error handling while at it. Signed-off-by: Kir Kolyshkin <[email protected]>
> test/main.go:33:49: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
> return fmt.Errorf("can't open image dir: %v", err)
> ^
> test/main.go:55:38: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
> return fmt.Errorf("dump fail: %v", err)
> ^
Note that %w requires go >= 1.13
(https://blog.golang.org/go1.13-errors).
Signed-off-by: Kir Kolyshkin <[email protected]>
> phaul/images.go:14:48: preparePhaulImages - result 1 (error) is always nil (unparam)
> func preparePhaulImages(wdir string) (*images, error) {
> ^
Signed-off-by: Kir Kolyshkin <[email protected]>
1. Skip go code which is autogenerated. 2. In addition to the default set of enabled linters, enable some more, from the following presets: - bugs - performance - unused - format This actually enables the following additional linters: - asciicheck - bodyclose - errorlint - exhaustive - exportloopref - gofmt - gofumpt - goimports - gosec (gas) - makezero - maligned - noctx - prealloc - rowserrcheck - scopelint - sqlclosecheck - unparam Signed-off-by: Kir Kolyshkin <[email protected]>
|
(CI for 1.16-rc1 is broken as it was removed; fix in #47) |
rst0git
approved these changes
Feb 17, 2021
|
@rst0git Do you want to tag the new release? 5.0.0? |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This
go mod tidyverify job to GHA;See individual commits for more details.
Description for changelog: