Skip to content

feat: bump bitswap message package to v1#212

Merged
guseggert merged 1 commit intomainfrom
feat/bitswap-msg-v1
Apr 3, 2023
Merged

feat: bump bitswap message package to v1#212
guseggert merged 1 commit intomainfrom
feat/bitswap-msg-v1

Conversation

@guseggert
Copy link
Contributor

@guseggert guseggert commented Mar 16, 2023 •
edited
Loading

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.

@guseggert
feat: bump bitswap message package to v1
16aae8a 
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 •
edited
Loading

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 •
edited
Loading

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
Jorropo
Jorropo approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@Jorropo Jorropo left a comment •
edited
Loading

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.

@guseggert guseggert merged commit 0e51f8a into main Apr 3, 2023
@guseggert guseggert deleted the feat/bitswap-msg-v1 branch April 3, 2023 13:48
This was referenced May 29, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jorropo Jorropo Jorropo approved these changes

Assignees

@guseggert guseggert

Labels

None yet

Projects

No open projects
IPFS Shipyard Team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@guseggert @Jorropo