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.

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

feat: bump bitswap message package to v1 #212

Merged
merged 1 commit into from
Apr 3, 2023
Merged

Conversation

guseggert
Copy link
Contributor

@guseggert guseggert commented Mar 16, 2023

This resolves issues registering protobufs from bitswap in go-libipfs when protobufs from go-bitswap are also registered in the same process.

We should really get off of gogoproto, the whole project is deprecated, but that is a larger effort.

This resolves issues registering protobufs from bitswap in
go-libipfs when protobufs from go-bitswap are also registered in the
same process.
@guseggert guseggert force-pushed the feat/bitswap-msg-v1 branch from 3c2dfea to 16aae8a Compare March 16, 2023 19:03
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #212 (16aae8a) into main (124ae4b) will increase coverage by 1.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
+ Coverage   30.01%   31.18%   +1.17%     
==========================================
  Files         101      101              
  Lines       11312    11499     +187     
==========================================
+ Hits         3395     3586     +191     
+ Misses       7549     7540       -9     
- Partials      368      373       +5     
Impacted Files Coverage Δ
bitswap/message/pb/message.pb.go 29.73% <100.00%> (ø)

... and 2 files with indirect coverage changes

@guseggert
Copy link
Contributor Author

guseggert commented Mar 16, 2023

I tested this as follows:

import (
	"fmt"
	"testing"

	"github.com/ipfs/go-bitswap/message"
	libipfsmsg "github.com/ipfs/go-libipfs/bitswap/message"
)

func TestThing(t *testing.T) {
	msg1 := message.New(true)
	msg2 := libipfsmsg.New(true)
	fmt.Printf("%s\n%s\n", msg1, msg2)
}

prior to this change, that panics like we have seen

go test -v -count=1 -run='TestThing' .
panic: proto: duplicate enum registered: bitswap.message.pb.Message_BlockPresenceType

goroutine 1 [running]:
github.com/gogo/protobuf/proto.RegisterEnum(...)
	/home/gus/.asdf/installs/golang/1.19.3/packages/pkg/mod/github.com/gogo/[email protected]/proto/properties.go:520
github.com/ipfs/go-libipfs/bitswap/message/pb.init.0()
	/home/gus/.asdf/installs/golang/1.19.3/packages/pkg/mod/github.com/ipfs/[email protected]/bitswap/message/pb/message.pb.go:371 +0x35a
FAIL

After the change it succeeds:

go test -v -count=1 -run='TestThing' .
=== RUN   TestThing
&{%!s(bool=true) map[] map[] map[] %!s(int32=0)}
&{%!s(bool=true) map[] map[] map[] %!s(int32=0)}
--- PASS: TestThing (0.00s)
PASS

@guseggert guseggert self-assigned this Mar 16, 2023
@guseggert guseggert marked this pull request as ready for review March 16, 2023 19:21
@guseggert guseggert requested a review from a team as a code owner March 16, 2023 19:21
@BigLep BigLep requested a review from Jorropo March 19, 2023 14:53
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

This is stupid to begin with, I don't like it, but it works and I don't think it's gonna break anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants