search

libp2p / go-libp2p-kad-dht

A Kademlia DHT implementation on go-libp2p
https://github.com/libp2p/specs/tree/master/kad-dht
MIT License
516 stars 221 forks source link

v2: upgrade build to include go1.21 #890

Closed iand closed 9 months ago

iand commented 10 months ago

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 commented 10 months ago

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

iand commented 10 months ago

The windows failures look real.

dennis-tra commented 10 months ago

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

dennis-tra commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.