v2: upgrade build to include go1.21 #890

iand · 2023-09-11T09:18:44Z

Tests pass locally with Go 1.20 and Go 1.21.0

The current UCI workflows don't support using different versions of Go in the same repo. To run the standard workflows we would need to upgrade v1 of go-libp2p-kad-dht to Go 1.21 compatible dependencies (due to quic-go refusing to build with Go 1.21 without an upgrade).

For now, we just want clean builds in v2 without touching v1 yet. This PR gives us that.

I've copied the standard UCI workflows from https://github.com/pl-strflt/uci/ and altered them to pass a working directory to the https://github.com/protocol/multiple-go-modules action. This restricts the checks and tests to run only on the v2 directory. This means that version 1 is not being tested, which is fine while this is on the v2-develop branch, but we will need to revisit when v2-develop is merged into master.

iand · 2023-09-11T10:21:41Z

CI is having trouble with different go.mods in the root of the repo and v2

iand · 2023-09-13T11:37:23Z

The windows failures look real.

dennis-tra · 2023-09-13T14:25:36Z

v2/config.go

@@ -178,7 +178,7 @@ type Config struct {
 // instantiate a fully functional [DHT] client. All fields that are nil require
 // some additional information to instantiate. The default values for these
 // fields come from separate top-level methods prefixed with Default.
-func DefaultConfig() *Config {
+func DefaultConfig() (*Config, error) {


I guess this is a result from a merge. We don't need an error return value here. This is the source for the comparatively large diff here.

I've removed in the next PR in the chain #887

Will remove here though - didn't realise there were so many

v2/go.mod

dennis-tra · 2023-09-13T14:29:24Z

Most of the diff is just the change that DefaultConfig returns an error now. I would remove that before merging

dennis-tra · 2023-09-13T14:46:01Z

I have no idea why the windows test is failing. It looks like the records receivedAt timestamp isn't parsed/set incorrectly.

In test TestDHT_handleGetValue_ipns_max_age_exceeded_in_datastore the returned response has a record attached to it although this can only happen if the record in the datastore is still valid. This cannot be the case because we set the MaxRecordAge to 0. A record can only be attached to the response if this condition is true. Assuming there are no errors this means r.cfg.clk.Since(receivedAt) is <= 0. This can only happen if either the clock is malconfigured or the receiveAt timestamp is set/parsed incorrectly 🤔 weird that it works with the other two OS's...

iand · 2023-09-13T14:47:32Z

Most of the diff is just the change that DefaultConfig returns an error now. I would remove that before merging

Hadn't noticed these. They must be from the merge up from v2-develop this morning

iand · 2023-09-13T14:52:24Z

I have no idea why the windows test is failing. It looks like the records receivedAt timestamp isn't parsed/set incorrectly.

In test TestDHT_handleGetValue_ipns_max_age_exceeded_in_datastore the returned response has a record attached to it although this can only happen if the record in the datastore is still valid. This cannot be the case because we set the MaxRecordAge to 0. A record can only be attached to the response if this condition is true. Assuming there are no errors this means r.cfg.clk.Since(receivedAt) is <= 0. This can only happen if either the clock is malconfigured or the receiveAt timestamp is set/parsed incorrectly 🤔 weird that it works with the other two OS's...

I know windows has or had a different clock granularity so maybe that is having an effect? This is where the mock clock would actually help.

marten-seemann · 2023-09-13T15:30:57Z

v2/go.mod

@@ -1,6 +1,6 @@
 module github.com/libp2p/go-libp2p-kad-dht/v2

-go 1.20
+go 1.21


This breaks the Go version compatibility promise adhered to by all of libp2p and PL Go packages in general. Is there a really good reason to do this?

Kubo/Boxo is expecting to upgrade within the next month.

go-libp2p-kad-dht v1 will remain at the current Go version for now. v2 is going to be experimental for a while yet. As long as we don't use any 1.21 features (or 1.20) then we can retain the 2 versions compatibility.

Kubo and Boxo have the same compatibility guarantee, regardless of the version they’re building the binaries with.

I’ve made significant efforts to support the latest two Go versions in quic-go, even when it would have been a lot easier to just drop Go 1.20. Our users (not only Kubo!) are relying on us keeping our compatibility promise.

There should be really, really good reasons (on the order of weeks of developer time) to justify breaking that promise.

We can drop it back to 1.20. The main point of this pr is to be ready to build with go 1.21, which we were not

Yes, that makes sense. What uCI is designed to do is test with the latest 2 Go versions (i.e. Go 1.20 and Go 1.21, at this point). This means that the version in go.mod should be kept at the lower version.

Done. We target the go 1.20 language but test with both go 1.20 and go 1.21. If you're happy with this can you approve the PR @marten-seemann ?

marten-seemann · 2023-09-14T05:16:49Z

.github/uci.yml

+  - .github/workflows/go-test.yml
+force: true # Configure whether Unified CI should overwrite existing workflows; defaults to false
+versions:
+  go: 1.21 # Configure what version of Go should be used; defaults to oldstable


This should include Go 1.20 as well.

This is actually no-op. uci.yaml isn't used by go-check or go-test (see https://filecoinproject.slack.com/archives/C03KLC57LKB/p1694518386936719?thread_ts=1694429677.253269&cid=C03KLC57LKB) Also we have customised both to pass in a v2 working directory, so this is irrelevant.

As mentions in the PR description the plan is to get working builds during v2 development and then integrate the CI with version 1 when we are closer to completion. Right now we are in the frustrating situation of not ever having clean builds.

* remove superfluous type conversion * add tiny example test * unexport type conversion helpers

v2: upgrade to go1.21

8cf5bbc

iand added the v2 All issues related to the v2 rewrite label Sep 11, 2023

iand requested a review from dennis-tra as a code owner September 11, 2023 09:18

iand added 2 commits September 11, 2023 11:07

Add uci config

b1b4661

Use newer uci actions

3997993

iand added 2 commits September 12, 2023 11:19

Fix merge conflict

77ca4ec

Use v2 working directory in actions

d7feea9

iand self-assigned this Sep 12, 2023

iand added 16 commits September 13, 2023 10:56

Set go-version input in actions

1299359

Set go-version input in actions

ce1fa35

Use go 1.20.8 in actions

52555e3

Use go 1.21.1 and relative working directory

9838121

Try default working directory on job

f08a1ea

Remove uci.yaml which is not supported yet

bf14d3e

Try default working directory on job

0e6e4d2

Try default working directory as input

318fdaa

Restore uci.yaml

c5768c2

Restore uci.yaml

576d604

Use modified go-check

54cc8e8

Use modified go-test

3826f9d

Fix go-test

f131d09

Fix go-test

b8662b9

Fix go-test

e94e01e

Merge branch 'v2-develop' into v2-go121

22ad479

iand requested a review from guillaumemichel September 13, 2023 11:42

dennis-tra reviewed Sep 13, 2023

View reviewed changes

v2/go.mod Outdated Show resolved Hide resolved

v2/go.mod Outdated Show resolved Hide resolved

iand added 2 commits September 13, 2023 15:40

Restore libp2p 0.30.0

960eca4

go mod tidy

60e8ff2

Remove nil error return from DefaultConfig

ebff55e

marten-seemann requested changes Sep 13, 2023

View reviewed changes

use mock clock for IPNS record generation (#894)

45dbf25

iand mentioned this pull request Sep 13, 2023

Test query interactions with routing table #887

Merged

Use MapDatastore for provider backend tests instead of leveldb (#896)

f607a79

marten-seemann reviewed Sep 14, 2023

View reviewed changes

dennis-tra and others added 5 commits September 14, 2023 11:36

revert some merge residuals

36328b1

style: minor coding clean up (#898)

961da3e

* remove superfluous type conversion * add tiny example test * unexport type conversion helpers

Target go language version 1.20 and add 1.20.8 to build matrix

f28c2fc

Target go language version 1.20 and add 1.20.8 to build matrix

7e13876

WIP

5665cda

iand changed the title ~~v2: upgrade to go1.21~~ v2: upgrade build to include go1.21 Sep 15, 2023

iand merged commit 97e4e02 into v2-develop Sep 16, 2023
11 checks passed

iand deleted the v2-go121 branch September 25, 2023 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2: upgrade build to include go1.21 #890

v2: upgrade build to include go1.21 #890

iand commented Sep 11, 2023 •

edited

Loading

iand commented Sep 11, 2023

iand commented Sep 13, 2023

dennis-tra Sep 13, 2023 •

edited

Loading

iand Sep 13, 2023

iand Sep 13, 2023

dennis-tra commented Sep 13, 2023

dennis-tra commented Sep 13, 2023 •

edited

Loading

iand commented Sep 13, 2023

iand commented Sep 13, 2023

marten-seemann Sep 13, 2023

iand Sep 13, 2023

marten-seemann Sep 13, 2023 •

edited

Loading

iand Sep 13, 2023

marten-seemann Sep 14, 2023

iand Sep 15, 2023

marten-seemann Sep 14, 2023

iand Sep 15, 2023

v2: upgrade build to include go1.21 #890

v2: upgrade build to include go1.21 #890

Conversation

iand commented Sep 11, 2023 • edited Loading

iand commented Sep 11, 2023

iand commented Sep 13, 2023

dennis-tra Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennis-tra commented Sep 13, 2023

dennis-tra commented Sep 13, 2023 • edited Loading

iand commented Sep 13, 2023

iand commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iand commented Sep 11, 2023 •

edited

Loading

dennis-tra Sep 13, 2023 •

edited

Loading

dennis-tra commented Sep 13, 2023 •

edited

Loading

marten-seemann Sep 13, 2023 •

edited

Loading