Closed MrDOS closed 1 year ago
Thank you for catching and fixing this @MrDOS π Closed&re-opened to trigger the CI.
Windows test were removed in #23 so it may be worth merging it first and rebasing this, to get a better CI results.
@MrDOS could you please rebase it on the latest master, now that win CI profile was removed?
Also, there seems to be a wired issue with linter CI profile, do you know what might be the reason? In #23 it seems to be working just fine.
level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package goarch: could not load export data: cannot import \"internal/goarch\" (unknown iexport format version 2), export data is newer version - update tool"
Hi @bzz, thanks for taking a look at this.
master
.I think the error from golangci-lint is because some of the newer dependencies ask for Go 1.18 (github.com/kevinburke/ssh_config, golang.org/x/exp, and github.com/neurosnap/sentences), and the older version of golangci-lint didn't support it properly across all of its linters. I suppose that even though this project isn't using Go 1.18, golangci-lint still tries to explore its dependencies.
I wasn't familiar with what happened when a Go module calls for an older version of Go than its dependencies. My understanding now is that, because dependencies are imported as source files and fed into the same compiler as the rest of the build, dependencies which ask for a newer version of Go will still work if the imported packages don't use any features from the newer language version. And I suppose the version number in go.mod
really is an ask, not a requirement β you decide what version of Go you're building with when you invoke the build process.
I tried building the application with Go 1.14, because that's the default that was used in the release action; but it failed due to the hard dependency on 1.16 introduced in #21. And Go 1.16 failed to build or test because of a hard dependency on 1.17 in the newest version of golang.org/x/sys. So even though a few of the dependencies (as noted) want 1.18, 1.17 seems like a safe enough target. The project builds and tests pass there. Particularly as this project is a library, you probably don't want to go too high to give consumers some margin to do their own updates. But 1.17 has been out for over a year, so it's hopefully sufficiently common-denominator enough by now.
Alternatively, I could roll dependency versions back down until I reach a set which are compatible with Go 1.16. Or, I could just not bump dependencies generally, and only specifically bump dependencies required to get the project to work properly with macOS ARM64. Your call. Either way, thanks for responding, and thanks for taking a look.
Thank you very much for digging deeper into it, your analysis makes perfect sense and Go 1.17 as a target sounds very reasonable.
I wonder, why Race CI profile takes 40min and rolling... π€
Oh, my local machine tests take 2min and indeed claimed runtime overhear of execution time by 2-20x
gives smth like that, although locally it's ~10min for me on M1.
Makes me wonder if that should be something optional rather than mandatory.. but I could not find Github equivalent of this Gitlab when: manual
feature π
Oh, cool, I'd never noticed that the golangci-lint action could take βlatestβ as a tool version. That's convenient.
It looks like v3 of the action wants the actions/setup-go
action to be explicitly invoked to install Go in the runner environment. I'll make that change.
Were there any other changes you wanted me to make?
It looks like v3 of the action wants the
actions/setup-go
action to be explicitly invoked to install Go in the runner environment. I'll make that change.
Oh, I see. But it also seems to be working alright now, doesn't it?
Were there any other changes you wanted me to make?
No, I've committed the suggestions and just forgot to change the review status π
But it also seems to be working alright now, doesn't it?
Yeah, I think it's just using whatever version of Go is on the PATH
in the runner. That's probably fine.
It does seem a little weird that the lint action runs tests, too, but also not actively harmful.
Thanks for the green checkmark! I'm happy with this if you're happy with this.
It does seem a little weird that the lint action runs tests, too
Thank you for bringing this up, indeed, this does not seem to be necessary. On the second thought, not relying on container's go version may be a good idea too.
Thank you very much for the improvements! Will merge and πͺ a new release tomorrow.
@bzz Happy new year! Friendly reminder, would you mind to create a release that includes these changes?
At risk of burying the lede, the important change here is bumping
golang.org/x/sys
, because this adds native support for Darwin on ARM64. With the old version, building on an Apple Silicon Mac fails:With this new version, the build succeeds, and produces a useful executable.
While I was making this change anyway, I thought it would be useful to attempt to bump other dependencies.
github.com/ekzhu/minhash-lsh
.gonum.org/v1/gonum
past v0.8.2 because of a breaking change in v0.9.0. See the commit message of f4552ce959319c9bf0da4475678457fd317a98f2 for details.