Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2: upgrade build to include go1.21 #890

Merged
merged 31 commits into from
Sep 16, 2023
Merged

v2: upgrade build to include go1.21 #890

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8cf5bbc
v2: upgrade to go1.21
iand Sep 11, 2023
b1b4661
Add uci config
iand Sep 11, 2023
3997993
Use newer uci actions
iand Sep 11, 2023
77ca4ec
Fix merge conflict
iand Sep 12, 2023
d7feea9
Use v2 working directory in actions
iand Sep 12, 2023
1299359
Set go-version input in actions
iand Sep 13, 2023
ce1fa35
Set go-version input in actions
iand Sep 13, 2023
52555e3
Use go 1.20.8 in actions
iand Sep 13, 2023
9838121
Use go 1.21.1 and relative working directory
iand Sep 13, 2023
f08a1ea
Try default working directory on job
iand Sep 13, 2023
bf14d3e
Remove uci.yaml which is not supported yet
iand Sep 13, 2023
0e6e4d2
Try default working directory on job
iand Sep 13, 2023
318fdaa
Try default working directory as input
iand Sep 13, 2023
c5768c2
Restore uci.yaml
iand Sep 13, 2023
576d604
Restore uci.yaml
iand Sep 13, 2023
54cc8e8
Use modified go-check
iand Sep 13, 2023
3826f9d
Use modified go-test
iand Sep 13, 2023
f131d09
Fix go-test
iand Sep 13, 2023
b8662b9
Fix go-test
iand Sep 13, 2023
e94e01e
Fix go-test
iand Sep 13, 2023
22ad479
Merge branch 'v2-develop' into v2-go121
iand Sep 13, 2023
960eca4
Restore libp2p 0.30.0
iand Sep 13, 2023
60e8ff2
go mod tidy
iand Sep 13, 2023
ebff55e
Remove nil error return from DefaultConfig
iand Sep 13, 2023
45dbf25
use mock clock for IPNS record generation (#894)
dennis-tra Sep 13, 2023
f607a79
Use MapDatastore for provider backend tests instead of leveldb (#896)
dennis-tra Sep 13, 2023
36328b1
revert some merge residuals
dennis-tra Sep 14, 2023
961da3e
style: minor coding clean up (#898)
dennis-tra Sep 15, 2023
f28c2fc
Target go language version 1.20 and add 1.20.8 to build matrix
iand Sep 15, 2023
7e13876
Target go language version 1.20 and add 1.20.8 to build matrix
iand Sep 15, 2023
5665cda
WIP
iand Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/uci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
files: # Configure what Unified CI templates should be used for your repository; defaults to primary language default fileset
- .github/workflows/go-check.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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include Go 1.20 as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

15 changes: 10 additions & 5 deletions v2/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"github.com/libp2p/go-libp2p/core/protocol"
ma "github.com/multiformats/go-multiaddr"
manet "github.com/multiformats/go-multiaddr/net"
"github.com/plprobelab/go-kademlia/coord"
"github.com/plprobelab/go-kademlia/kad"
"github.com/plprobelab/go-kademlia/key"
"github.com/plprobelab/go-kademlia/routing"
Expand All @@ -21,6 +20,8 @@ import (
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap/exp/zapslog"
"golang.org/x/exp/slog"

"github.com/libp2p/go-libp2p-kad-dht/v2/coord"
)

// ServiceName is used to scope incoming streams for the resource manager.
Expand Down Expand Up @@ -112,7 +113,7 @@ type Config struct {
Mode ModeOpt

// Kademlia holds the configuration of the underlying Kademlia implementation.
Kademlia *coord.Config
Kademlia *coord.CoordinatorConfig

// BucketSize determines the number of closer peers to return
BucketSize int
Expand Down Expand Up @@ -178,11 +179,15 @@ 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) {
Copy link
Contributor

@dennis-tra dennis-tra Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

coordConfig, err := coord.DefaultCoordinatorConfig()
if err != nil {
return nil, err
}
return &Config{
Clock: clock.New(),
Mode: ModeOptAutoClient,
Kademlia: coord.DefaultConfig(),
Kademlia: coordConfig,
BucketSize: 20, // MAGIC
ProtocolID: ProtocolIPFS,
RoutingTable: nil, // nil because a routing table requires information about the local node. triert.TrieRT will be used if this field is nil.
Expand All @@ -193,7 +198,7 @@ func DefaultConfig() *Config {
AddressFilter: AddrFilterPrivate,
MeterProvider: otel.GetMeterProvider(),
TracerProvider: otel.GetTracerProvider(),
}
}, nil
}

// DefaultRoutingTable returns a triert.TrieRT routing table. This routing table
Expand Down
42 changes: 28 additions & 14 deletions v2/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,48 +9,56 @@ import (

func TestConfig_Validate(t *testing.T) {
t.Run("happy path", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
assert.NoError(t, cfg.Validate())
})

t.Run("invalid mode", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.Mode = "invalid"
assert.Error(t, cfg.Validate())
})

t.Run("nil Kademlia configuration", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.Kademlia = nil
assert.Error(t, cfg.Validate())
})

t.Run("invalid Kademlia configuration", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.Kademlia.Clock = nil
assert.Error(t, cfg.Validate())
})

t.Run("empty protocol", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.ProtocolID = ""
assert.Error(t, cfg.Validate())
})

t.Run("nil logger", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.Logger = nil
assert.Error(t, cfg.Validate())
})

t.Run("0 stream idle timeout", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.TimeoutStreamIdle = time.Duration(0)
assert.Error(t, cfg.Validate())
})

t.Run("negative stream idle timeout", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.TimeoutStreamIdle = time.Duration(-1)
assert.Error(t, cfg.Validate())
})
Expand All @@ -61,14 +69,16 @@ func TestConfig_Validate(t *testing.T) {
// If the Backends map is empty and the IPFS protocol is configured,
// we automatically populate the DHT backends for these record
// types.
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.ProtocolID = ProtocolIPFS
cfg.Backends["another"] = &RecordBackend{}
assert.Error(t, cfg.Validate())
})

t.Run("additional backends for ipfs protocol", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.ProtocolID = ProtocolIPFS
cfg.Backends[namespaceProviders] = &RecordBackend{}
cfg.Backends[namespaceIPNS] = &RecordBackend{}
Expand All @@ -78,25 +88,29 @@ func TestConfig_Validate(t *testing.T) {
})

t.Run("nil address filter", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.AddressFilter = nil
assert.Error(t, cfg.Validate())
})

t.Run("nil meter provider", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.MeterProvider = nil
assert.Error(t, cfg.Validate())
})

t.Run("nil tracer provider", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.TracerProvider = nil
assert.Error(t, cfg.Validate())
})

t.Run("nil clock", func(t *testing.T) {
cfg := DefaultConfig()
cfg, err := DefaultConfig()
assert.NoError(t, err)
cfg.Clock = nil
assert.Error(t, cfg.Validate())
})
Expand Down
5 changes: 4 additions & 1 deletion v2/dht.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ func New(h host.Host, cfg *Config) (*DHT, error) {
var err error

if cfg == nil {
cfg = DefaultConfig()
cfg, err = DefaultConfig()
if err != nil {
return nil, fmt.Errorf("default config: %w", err)
}
} else if err = cfg.Validate(); err != nil {
return nil, fmt.Errorf("validate DHT config: %w", err)
}
Expand Down
8 changes: 5 additions & 3 deletions v2/dht_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ func TestNew(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := DefaultConfig()
c, err := DefaultConfig()
require.NoError(t, err)
d, err := New(h, c)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -94,7 +95,8 @@ func expectEventType(t *testing.T, ctx context.Context, events <-chan coord.Rout
func TestAddAddresses(t *testing.T) {
ctx := kadtest.CtxShort(t)

localCfg := DefaultConfig()
localCfg, err := DefaultConfig()
assert.NoError(t, err)

local := newClientDht(t, localCfg)

Expand All @@ -104,7 +106,7 @@ func TestAddAddresses(t *testing.T) {
fillRoutingTable(t, remote, 1)

// local routing table should not contain the node
_, err := local.kad.GetNode(ctx, remote.host.ID())
_, err = local.kad.GetNode(ctx, remote.host.ID())
require.ErrorIs(t, err, coord.ErrNodeNotFound)

remoteAddrInfo := peer.AddrInfo{
Expand Down
32 changes: 17 additions & 15 deletions v2/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
module github.com/libp2p/go-libp2p-kad-dht/v2

go 1.20
go 1.21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@marten-seemann marten-seemann Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?


replace github.com/multiformats/go-multistream => github.com/multiformats/go-multistream v0.4.1
dennis-tra marked this conversation as resolved.
Show resolved Hide resolved

require (
github.com/benbjohnson/clock v1.3.5
Expand All @@ -10,12 +12,12 @@ require (
github.com/ipfs/go-datastore v0.6.1-0.20230901172804-1caa2449ed7c
github.com/ipfs/go-ds-leveldb v0.5.0
github.com/ipfs/go-log/v2 v2.5.1
github.com/libp2p/go-libp2p v0.30.0
github.com/libp2p/go-libp2p v0.29.1-0.20230817072656-d2398ee4f251
dennis-tra marked this conversation as resolved.
Show resolved Hide resolved
github.com/libp2p/go-libp2p-record v0.2.0
github.com/libp2p/go-msgio v0.3.0
github.com/multiformats/go-base32 v0.1.0
github.com/multiformats/go-multiaddr v0.11.0
github.com/plprobelab/go-kademlia v0.0.0-20230901130940-286ab4ceca60
github.com/plprobelab/go-kademlia v0.0.0-20230911085009-18d957853c57
github.com/stretchr/testify v1.8.4
go.opentelemetry.io/otel v1.17.0
go.opentelemetry.io/otel/exporters/jaeger v1.16.0
Expand All @@ -24,7 +26,7 @@ require (
go.opentelemetry.io/otel/sdk/metric v0.40.0
go.opentelemetry.io/otel/trace v1.17.0
go.uber.org/zap/exp v0.1.0
golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63
golang.org/x/exp v0.0.0-20230905200255-921286631fa9
google.golang.org/protobuf v1.31.0
)

Expand All @@ -49,10 +51,10 @@ require (
github.com/golang/protobuf v1.5.3 // indirect
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db // indirect
github.com/google/gopacket v1.1.19 // indirect
github.com/google/pprof v0.0.0-20230817174616-7a8ec2ada47b // indirect
github.com/google/pprof v0.0.0-20230907193218-d3ddc7976beb // indirect
github.com/google/uuid v1.3.0 // indirect
github.com/gorilla/websocket v1.5.0 // indirect
github.com/huin/goupnp v1.2.0 // indirect
github.com/huin/goupnp v1.3.0 // indirect
github.com/ipld/go-ipld-prime v0.21.0 // indirect
github.com/jackpal/go-nat-pmp v1.0.2 // indirect
github.com/jbenet/go-temp-err-catcher v0.1.0 // indirect
Expand Down Expand Up @@ -85,7 +87,7 @@ require (
github.com/multiformats/go-multistream v0.4.1 // indirect
github.com/multiformats/go-varint v0.0.7 // indirect
github.com/nxadm/tail v1.4.8 // indirect
github.com/onsi/ginkgo/v2 v2.11.0 // indirect
github.com/onsi/ginkgo/v2 v2.12.0 // indirect
github.com/opencontainers/runtime-spec v1.1.0 // indirect
github.com/pbnjay/memory v0.0.0-20210728143218-7b4eea64cf58 // indirect
github.com/pkg/errors v0.9.1 // indirect
Expand All @@ -94,10 +96,10 @@ require (
github.com/prometheus/client_golang v1.16.0 // indirect
github.com/prometheus/client_model v0.4.0 // indirect
github.com/prometheus/common v0.44.0 // indirect
github.com/prometheus/procfs v0.11.0 // indirect
github.com/prometheus/procfs v0.11.1 // indirect
github.com/quic-go/qpack v0.4.0 // indirect
github.com/quic-go/qtls-go1-20 v0.3.2 // indirect
github.com/quic-go/quic-go v0.37.6 // indirect
github.com/quic-go/qtls-go1-20 v0.3.4 // indirect
github.com/quic-go/quic-go v0.38.1 // indirect
github.com/quic-go/webtransport-go v0.5.3 // indirect
github.com/raulk/go-watchdog v1.3.0 // indirect
github.com/spaolacci/murmur3 v1.1.0 // indirect
Expand All @@ -106,13 +108,13 @@ require (
go.uber.org/fx v1.20.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.25.0 // indirect
golang.org/x/crypto v0.12.0 // indirect
golang.org/x/crypto v0.13.0 // indirect
golang.org/x/mod v0.12.0 // indirect
golang.org/x/net v0.14.0 // indirect
golang.org/x/net v0.15.0 // indirect
golang.org/x/sync v0.3.0 // indirect
golang.org/x/sys v0.11.0 // indirect
golang.org/x/text v0.12.0 // indirect
golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846 // indirect
golang.org/x/sys v0.12.0 // indirect
golang.org/x/text v0.13.0 // indirect
golang.org/x/tools v0.13.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
lukechampine.com/blake3 v1.2.1 // indirect
)
Loading