trezor / trezord-go

:smiling_imp: Trezor Communication Daemon (written in Go)
GNU Lesser General Public License v3.0
244 stars 146 forks source link

Switch to Go modules #198

Closed onionltd closed 3 years ago

onionltd commented 3 years ago

Go Modules is the preferred way to version packages since Go 1.11.

prusnak commented 3 years ago

Thank you very much! I planned to do this for long time!

Is the file vendor/modules.txt really necessary? Shouldn't we just remove the whole vendor folder and be done with it?

onionltd commented 3 years ago

vendor directory may still be needed for older versions of Go (everything below 1.11).

The README states that trezord-go requires Go >=1.6, however, I found that the build works only with Go >= 1.7 due to context package used in core/core.go, which was introduced more recently. More importantly, travis jobs build with Go 1.11+, omitting older versions. So there's definitely some inconsistency.

prusnak commented 3 years ago

I don't plan to support anything older than 1.11, so let's nuke the vendor dir completely. Can you please also adjust README to say we support only Go 1.11+? Thanks!

onionltd commented 3 years ago

I've removed the vendor directory and updated the README.

I attempted to upgrade the requirements to their latest versions, unfortunately after the upgrade, the build passes only with Go >=1.12. Should we change the minimum supported version to Go 1.12 or leave it as it is, running with outdated packages?

prusnak commented 3 years ago

Should we change the minimum supported version to Go 1.12

Yes, I am fine with this change.

onionltd commented 3 years ago

I pushed the changes but there are two failed jobs. First the gometalinter, it fails due to missing dependency. I noticed the project is obsolete and there's an open PR #153 to replace it.

The other failed job is windows build. It fails due to some weird C error. I noticed the same error appears on build for the latest master.

prusnak commented 3 years ago

First the gometalinter, it fails due to missing dependency.

Please fix this by adding the dependency for now. I'll address the PR #153 later.

I noticed the same error appears on build for the latest master.

That's not right, I fixed this in master meanwhile.

I am changing the target of this branch to the next branch (which has the same HEAD as the current master with the fix) where we will brew the next version with Go modules support (I prefer to keep master stable).

Will you please rebase this PR on top of the current next? This should fix the "weird C error".

onionltd commented 3 years ago

Please fix this by adding the dependency for now. I'll address the PR #153 later.

The missing/broken dependency is a part of gometalinter. I'm not sure why the changes cause the job to fail, I tried with GO111MODULE set to auto|on.The result were two different errors. As far as i'm concerned, the package is fubar.

Will you please rebase this PR on top of the current next? This should fix the "weird C error".

Done.

prusnak commented 3 years ago

I rebased the PR #153 into the next branch - let's see how that works in the CI.

onionltd commented 3 years ago

I rebased the PR #153 into the next branch - let's see how that works in the CI.

I extracted the core changes from #153, updated them to fit the latest release of golangci-lint and pushed all of it here: https://github.com/onionltd/trezord-go/tree/golangci-lint

What's the best way forward? IMHO drop the commits introduced by merge of #153 and merge changes from golangci-lint instead. The CI is going to fail but this time it will fail on linter errors. I'll fix those next.

prusnak commented 3 years ago

I pushed changes from your golangci-lint branch into next2 branch.

Let's see what the CI says ... You are welcome to sent any subsequent lint fixes to that branch.

prusnak commented 3 years ago

I changed the base branch of this PR to next2 - can you please rebase this PR?

onionltd commented 3 years ago

It's done.

prusnak commented 3 years ago

Thank you very much! I'll merge this into master after 2.0.30 is released, which will be hopefully soon.

prusnak commented 3 years ago

Merged into master (even before 2.0.30 which will come soon), thanks again!