More cleanups #38
More cleanups #38
Conversation
|
CI seems to have network problems. |
| go test -run xxxxxMatchNothingxxxxx ./... >/dev/null # check if tests build properly | ||
| ./tools/golangci-lint run --fix | ||
| go mod tidy |
There was a problem hiding this comment.
Do we really want to run all of this when building? I think it makes more sense to have a separated target that is run by the CI and by developers when they want to check the style.
There was a problem hiding this comment.
Linter isn't about the style/formatting only. It finds unused code, ignored errors and so on. So I prefer to run it every time. Also, it doesn't take a lot of time anyway.
There was a problem hiding this comment.
My point is, if you are developing and just one to perform a quick compilation without caring about other details, should we run the lint? Won't it be too annoying to have the lint in between when you only want to compile to test a change you did?
There was a problem hiding this comment.
It mostly wasn't annoying in my experience. We did the same in otel-go and nebraska, and it was just fine. The linter is run with the --fix flag, which tries to fix the issues it finds automatically.
The only annoyance was when there were bugs in the linter, but those get fixed rather quickly.
There was a problem hiding this comment.
Ah right, it has the --fix option, it should be good then.
Ah, yeah. If you will see the list of commits, then you'll notice a revert commit.
I haven't yet got around too fix the issues, hence I just dropped the |
golangci-lint requires a comment about blank imports. I copied a comment from k8s client-go code.
This is to placate golangci-lint (it does not like the implicitly ignored errors). Most of the places where we ignore errors are suspicious, so I wrote a TODO comment there.
Seems like this functionality got moved to the scripts that should be run as a part of the build process.
The linters should clean up the imports and the code.
It's basically the same as the default Makefile target (`all`), but it also checks if the tree is clean after the build. This should catch an issue when the generated files need updating, or if the code was not properly formatted.
The scripts now take a parameter being an output file. This will make it possible to use them in the `go:generate` comments. We also filter out an ID line from output params - it is likely to differ from kernel to kernel and we don't care about it. Other than that - minor cleanups, like using \t instead of literal tabs.
I'd say that the scripts are also the tools, so let's keep the helper stuff in a single place.
This step was done manually, but the follow up commit will make it automatic.
Not sure about this change - it will generate different results on older kernels. It also may trip the CI if the CI is running on older kernel than the developer's machine.
This reverts commit ee6fe99. The CI doesn't like it. We will need to run go generate under sudo I think.
krnowak
force-pushed
the
krnowak/lint-rest
branch
from
79c4ec9 to
f1362b8
Compare
|
I wonder if there is a way to get the information from the kernel sources or the kernel headers instead of /sys... That would have the benefit that the code generator could run from a container with a specific version of the kernel package installed, and that would not be dependant on the kernel used by the CI. But I agree this does not have to be fixed in this PR. |
This PR enables golangci-lint linter by default now. To do that I had to placate it first. Some of the ignored errors are under TODO as it wasn't clear for me what is the path forward. The CI will fail the tree is not clean after building and linting.
Other than that, I have changed that way the syscall stuff is generated. We use
go generateto do it for us. One change I'm not sure about it running go generate on every build - this may trip us up during CI as the CI will likely run under a kernel that's older than the one on the developer machine.I planned cleaning up the traceloop builder dockerfile, but apparently clang does not like the kernel headers in fedora 32, so I left that to be done later.