Fix golangci-lint issues reported on different OS

[albertony]

What is the purpose of this change?

Fixes the following issues from running golangci-lint on Windows, as well as a couple of more:

cmd\serve\nfs\nfs_unsupported.go:13:30: var-declaration: should drop = nil from declaration of var Command; it is the zero value (revive)
var Command *cobra.Command = nil
                             ^
cmd\serve\nfs\nfs_unsupported.go:12:1: exported: comment on exported var Command should be of the form "Command ..." (revive)
// For unsupported platforms we just put nil
^
lib\buildinfo\osversion_windows.go:100:6: func `getRegistryVersionInt` is unused (unused)
func getRegistryVersionInt(name string) int {
     ^
lib\file\mkdir_windows_test.go:38:15: Error return value of `f.Close` is not checked (errcheck)
        defer f.Close()
                     ^
lib\file\preallocate_windows.go:95:32: exported: comment on exported const FSCTL_SET_SPARSE should be of the form "FSCTL_SET_SPARSE ..." (revive)
        FSCTL_SET_SPARSE = 0x000900c4 // Control code to set or clears the FILE_ATTRIBUTE_SPARSE_FILE attribute of a file.
                                      ^
lib\file\preallocate_windows.go:59:10: unnecessary conversion (unconvert)
                uintptr(out.Fd()),
                       ^
lib\file\preallocate_windows.go:62:10: unnecessary conversion (unconvert)
                uintptr(unsafe.Sizeof(fsSizeInfo)),
                       ^
lib\file\preallocate_windows.go:78:10: unnecessary conversion (unconvert)
                uintptr(out.Fd()),
                       ^
lib\file\preallocate_windows.go:81:10: unnecessary conversion (unconvert)
                uintptr(unsafe.Sizeof(allocInfo)),
                       ^
lib\file\preallocate_windows.go:85:25: unnecessary conversion (unconvert)
                if e1 == syscall.Errno(windows.ERROR_DISK_FULL) || e1 == syscall.Errno(windows.ERROR_HANDLE_DISK_FULL) {
                                      ^
lib\terminal\hidden_windows.go:17:19: Error return value of `showWindow.Call` is not checked (errcheck)
                        showWindow.Call(hwnd, 0)
                                       ^
fs\fserrors\retriable_errors_windows.go:27:2: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
        WSAHOST_NOT_FOUND syscall.Errno = 11001
        ^
fs\fserrors\retriable_errors_windows.go:28:2: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
        WSATRY_AGAIN      syscall.Errno = 11002
        ^
backend\local\remove_windows.go:15:2: exported: exported const ERROR_SHARING_VIOLATION should have comment (or a comment on this block) or be unexported (revive)
        ERROR_SHARING_VIOLATION syscall.Errno = 32
        ^
fs\accounting\token_bucket.go:43:20: func `(*buckets)._isOff` is unused (unused)
func (bs *buckets) _isOff() bool {
                   ^
fs\log\redirect_stderr_windows.go:24:15: SA1019: syscall.Syscall has been deprecated since Go 1.18: Use SyscallN instead. (staticcheck)
        r0, _, e1 := syscall.Syscall(procSetStdHandle.Addr(), 2, uintptr(stdhandle), uintptr(handle), 0)

Was the change discussed in an issue or in the forum before?

Checklist

• I have read the contribution guidelines.
• I have added tests for all changes in this PR if appropriate.
• I have added documentation for the changes if appropriate.
• All commit messages are in house style.
• I'm done, this Pull Request is ready for review :-)

[ncw]

Do you think we should run golangci-lint on windows/macOS too?

Actually I don't think we need to as this seems to work

GOOS=windows golangci-lint run ./...

We can probably wedge that into the lint workflow somehow...

[albertony]

Maybe we could.. However the running time maybe an issue? At least if we end up running it once for each of the supported GOOS values... PR checks already takes some 15-20 minutes, which is considerable. The linting something above 40s of that it seems, and of that again e.g. 11s for installing golangci-lint in the runner and 31s for running it. Running it multiple times in the same agent will hopefully run faster, assuming its clever enough to reuse cached results. Worth a little experiment, yes.

[albertony]

I also noticed the following golangci-options while digging a bit into it earlier today, if it slows the checks down too much it could perhaps be an option to configure it to only check actual changed code when triggered by PRs? I haven't tested how it works, though.

  -n, --new                            Show only new issues: if there are unstaged changes or untracked files, only those changes are analyzed, else only changes in HEAD~ are analyzed.
                                       It's a super-useful option for integration of golangci-lint into existing large codebase.
                                       It's not practical to fix all existing issues at the moment of integration: much better to not allow issues in new code.
                                       For CI setups, prefer --new-from-rev=HEAD~, as --new can skip linting the current patch if any scripts generate unstaged files before golangci-lint runs.
      --new-from-rev REV               Show only new issues created after git revision REV

[albertony]

I've been thinking, some of the issues may not be from golangci-lint, actually. I ran it from within VSCode at one point, and then it reports some issues from gopls as well, so I can look into that and revert - if some of them looks closer to false positives.

[ncw]

Maybe we could.. However the running time maybe an issue? At least if we end up running it once for each of the supported GOOS values... PR checks already takes some 15-20 minutes, which is considerable. The linting something above 40s of that it seems, and of that again e.g. 11s for installing golangci-lint in the runner and 31s for running it.

The checks run in parallel so as long as we don't make the lint step take more that say 10 mins it won't affect the total running time.

I think the lint runs quite quickly so I don't think we'll need to enable the diff check.

You can always try the golangci lint binary from the command line to see if it matches vscode

[albertony]

I took a step back and re-ran the linting, and removed most of the fixes which were not from golangci-lint. Left a few of them, though, as I found they were quite unquestionable, as well as the go mod tidy commit.

The removed gopls reported issues was mainly unusedparams issues like this:

-func lChtimes(_ string, _ time.Time, _ time.Time) error {
+func lChtimes(name string, atime time.Time, mtime time.Time) error {
       // Does nothing
       return nil
}

I assume golangci-lint would have reported them as well, if we hadn't had this configuration:

rclone/.golangci.yml

Lines 91 to 92 in c09426b

	- name: unused-parameter
	disabled: true

Mainly because there would be a lot of such issues reported, probably.

[ncw]

Nice! What is up with netbsd and solaris I wonder?

I took a step back and re-ran the linting, and removed most of the fixes which were not from golangci-lint. Left a few of them, though, as I found they were quite unquestionable, as well as the go mod tidy commit.

The removed gopls reported issues was mainly unusedparams issues like this:
[...]
I assume golangci-lint would have reported them as well, if we hadn't had this configuration:

rclone/.golangci.yml

Lines 91 to 92 in c09426b

	- name: unused-parameter
	disabled: true

Mainly because there would be a lot of such issues reported, probably.

Can you get vscode to use the the golangci-lint config that we supply in the root of the rclone source repo?

I didn't enable unusedparams as in a project like rclone we implement the same interface many, many times and I think it is actually useful to keep the function definitions constant rather than change some of them to have _ in.

[albertony]

Can you get vscode to use the the golangci-lint config that we supply in the root of the rclone source repo?

Yes it does that. VSCode with go extension uses gopls for various language features and staticcheck specifically for linting by default, and its a simple setting to switch from staticcheck to golangci-lint. However the gopls, used for intellisense and a lot of things, also seem to do some lint-like checks. So its that gopls check that reports the unusedparams I mentioned. If I explicitely run the linting feature of VSCode it comes reports no issues. But when the gopls checks kick in they report additional issues, and all are mixed in a single "Problems" panel, thats why I "blamed" it on golangci-lint at first. I may look into the gopls vs lint settings at some point, but I think I'll just live with it for now..

I didn't enable unusedparams as in a project like rclone we implement the same interface many, many times and I think it is actually useful to keep the function definitions constant rather than change some of them to have _ in.

Yes, I concluded on that too, that parameter names may indeed add relevant information even if not used.

[albertony]

What is up with netbsd and solaris I wonder?

I don't know.. 😕 I suspect something to do with caching. In addition to the separate module cache step, which I suggest removing with #7779, the setup-go action has caching of modules and build output, and the golangci-lint action has caching of modules, build output - and lint results.

Edit: In the build logs (also existing builds) there are from time to time a lot of "Cannot open: File exists" entries which is related to caching as well. Many discussions on similar things over at https://github.com/golangci/golangci-lint-action.

Edit: I also added setup-go action to the lint job. The golangci-lint-action docs states:

v4.0.0+ requires an explicit setup-go installation step before using this action: uses: actions/setup-go@v5. The skip-go-installation option has been removed.

But it obviously worked without it as well... 🤔

[ncw]

What is up with netbsd and solaris I wonder?

I don't know.. 😕 I suspect something to do with caching. In addition to the separate module cache step, which I suggest removing with #7779, the setup-go action has caching of modules and build output, and the golangci-lint action has caching of modules, build output - and lint results.

I wonder if golangci-lint is coping with caching results for different architectures OK? You can set cache keys for github runs but not sure that is relevant here since everything is in one run.

Edit: In the build logs (also existing builds) there are from time to time a lot of "Cannot open: File exists" entries which is related to caching as well. Many discussions on similar things over at https://github.com/golangci/golangci-lint-action.

Can try skip-cache: true I suppose - that is suggested in an issue.

Edit: I also added setup-go action to the lint job. The golangci-lint-action docs states:

v4.0.0+ requires an explicit setup-go installation step before using this action: uses: actions/setup-go@v5. The skip-go-installation option has been removed.

But it obviously worked without it as well... 🤔

Odd! But your change sounds correct :-)

[albertony]

I just added a commit which I think fixes the issues by some small adjustments to build tags in a couple of tests etc. Seems that fixed it actually, so maybe not caching after all.

Unfortunately I also added linting on dragonfly, so that introduced a lot of errors again.. 😆 But not too hard to fix I think, with some more adjustments to build tags.

But I don't know for which GOOS we do want to run the linting? All we have build tags for in code (linux, windows, darwin, freebsd, netbsd, openbsd, dragonfly, plan9, js, solaris)? All that we build during the PR checks (linux, darwin, windows)? All that we have builds for on download page (linux, windows, darwin, freebsd, netbsd, openbsd, plan9, solaris - but not dragonfly or js)?

[albertony]

All green in https://github.com/rclone/rclone/actions/runs/8737234254, with goos linux, windows, darwin, freebsd, netbsd, openbsd, dragonfly, plan9, solaris, but skipped js as that fails with "Running error: context loading failed: no go files to analyze: running go mod tidy may solve the problem".

But that run was with all cache hits on golangci-lint-action caches. I have manually deleted the caches now, and re-running it to see how that behaves..

[albertony]

But that run was with all cache hits on golangci-lint-action caches. I have manually deleted the caches now, and re-running it to see how that behaves..

That worked too: https://github.com/rclone/rclone/actions/runs/8737533920/job/23974720264

However, in the automatic Post processing steps, where the cache saving happens, the first post-step that runs, which is for the last lint step that ran, "Post Code quality test (Solaris)", saves the cache:

Cache Size: ~12 MB (12671557 B)
Cache saved successfully
Saved cache for golangci-lint from paths '/home/runner/.cache/golangci-lint' in 4932ms

While the rest of the post steps does nothing - because all have the same cache key:

Failed to save: Unable to reserve cache with key golangci-lint.cache-2833-820432f836f6c171106fffde2baea2a6de7ef88e, another job may be creating this cache. More details: Cache already exists. Scope: refs/pull/7776/merge, Key: golangci-lint.cache-2833-820432f836f6c171106fffde2baea2a6de7ef88e, Version: a8bb4d051abedb2e113db79282817e72e10ab797944320637e9cb6fad8ce5a5c
Saved cache for golangci-lint from paths '/home/runner/.cache/golangci-lint' in 2237ms

Next I'm running yet another build: https://github.com/rclone/rclone/actions/runs/8738200411/job/23976751398
This will pick up the saved cache, download it and extract into place:

Cache Size: ~12 MB (12671557 B)
  /usr/bin/tar -xf /home/runner/work/_temp/9[15](https://github.com/rclone/rclone/actions/runs/8738200411/job/23976751398#step:4:16)02d34-5e6a-492d-acf5-a9db0301603e/cache.tzst -P -C /home/runner/work/rclone/rclone --use-compress-program unzstd
  Received 12671557 of 12671557 (100.0%), 12.1 MBs/sec
  Cache restored successfully
  Restored cache for golangci-lint from key 'golangci-lint.cache-2833-820432f836f6c[17](https://github.com/rclone/rclone/actions/runs/8738200411/job/23976751398#step:4:18)1106fffde2baea2a6de7ef88e' in 3989ms

And when done, the post steps does nothing:

Cache hit occurred on the primary key golangci-lint.cache-2833-820432f836f6c171106fffde2baea2a6de7ef88e, not saving cache.

And this will be done again for each of the lint steps, downloading the same cache, extracting over the existing identical one.

So to conclude:

• We can let golangci-lint-action cache the lint results as it does by default.
• With multiple golangci-lint-action steps in a row, the stored cache contains the results from all of them (all GOOS variants) in the same cache.
• This seem to work fine, although does not seem to be a use case explicitly considered, e.g.
  • When cache can be reused it will unnecessarily be restored over again for each step.
  • When cache could not be used then the linting results is attempted to be stored back for each step, but only the first succeeds.

Maybe there is a trick to force use of individual caches, however as long as this works we save cache storage (which is under pressure: "Approaching total cache storage limit (14.47 GB of 10 GB Used)", and some time (for cache upload at least, but it already repeats the cache restores so no change there).

So I think the remaining question is for which GOOS we should do the linting?

[albertony]

The current approach, repeating golangci-lint-action steps with different GOOS, is based on:

• make it possible to set the GOOS / GOARCH golangci/golangci-lint-action#742

[ncw]

I just added a commit which I think fixes the issues by some small adjustments to build tags in a couple of tests etc. Seems that fixed it actually, so maybe not caching after all.

:-)

Unfortunately I also added linting on dragonfly, so that introduced a lot of errors again.. 😆 But not too hard to fix I think, with some more adjustments to build tags.

:-(

But I don't know for which GOOS we do want to run the linting? All we have build tags for in code (linux, windows, darwin, freebsd, netbsd, openbsd, dragonfly, plan9, js, solaris)? All that we build during the PR checks (linux, darwin, windows)? All that we have builds for on download page (linux, windows, darwin, freebsd, netbsd, openbsd, plan9, solaris - but not dragonfly or js)?

Good question. I think linux, darwin, windows, freebsd will hit > 99% of the rclone users so any more we are into diminishing returns. Most of the build tags are used to disable features which don't build on a given OS. I think if you added openbsd to that list then you'd hit backend/local/metadata_unix.go and lib/diskusage/diskusage_openbsd.go which are the only files I could find which doesn't build under the first 4.

So if we can afford the runtime for 5 linters linux, darwin, windows, freebsd & openbsd will hit all the files.

If we only have 4 then I'd choose linux, darwin, windows, freebsd being the most popular OSes.

Will this stop on the first lint failure? The reason I ask that is that the lint action also puts comments on the PRs so if the user makes a normal sort of dev mistake then all the linters will pick it up and there will be N comments on the PR. So I think it should probably stop on the first linter to fail.

[albertony]

Sounds good.

Will this stop on the first lint failure?

No. I intended it not to.

all the linters will pick it up and there will be N comments on the PR.

But that is a good point. Maybe I can push a build error to see, but you're probably right, and then I agree it should abort on first linter.

[albertony]

Done:

• Aborting lint job after first failed (GOOS-specific) lint step
• Removed the unnecessary GOOS linters, leaving the big 5: linux, darwin, windows, freebsd & openbsd
  • Even when I had all the 9 linters they all finished within the time it took for all the builds, e.g. 11-13m, while mac_amd64 build may be as slow as 16-18m, so with 5 linters I think we can be pretty sure these finish before all the builds.

Unfortunately:

I just noticed that it seems the golangci-lint executions always takes about the same time now, like 1 minute or so, even when it reports it found a cache - in which case it previously executed like 5 seconds. Probably the changing of GOOS invalidates the cache in the sense that it needs to rebuild regardless. Not sure how easy that is to fix, I fear some custom caching hacks is needed, like completely disabling the automatic caching from both setup-go and golangci-lint-action and adding an explicit cache action step for each of the golangci-lint-action steps. The alternative is to run without cache, just disable it if it is true it does not have any real effect, and accept it being a bit slow as long as it is not limiting the entire build. Would be unfortunate, but exactly how much and how acceptable that would be I don't know? I will investigate this cache topic later, like tomorrow.

[albertony]

I think I've solved the last caching issue of the lint job.

To test I manually deleted relevant caches, and forced a build which did the entire lint job in about 6m (1m35s on each lint step), which includes saving a new cache. Then forced another build, and the entire lint job now took 46s (2s on each lint step). I think the current solution should be solid. Note that there may be some strange effects as long as this PR does the lint job differently than master and other PRs, because they may generate same cache keys and end up reusing each others caches, with unexpected content.

@ncw Let me know what you think of it now.

[ncw]

Looks perfect now - thank you for all your hard work on this.

Please merge :-)