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

WIP: update go-libp2p to v0.24.0 #9423

Merged
merged 7 commits into from
Dec 12, 2022
Merged

WIP: update go-libp2p to v0.24.0 #9423

merged 7 commits into from
Dec 12, 2022

Conversation

@marten-seemann marten-seemann marked this pull request as draft November 20, 2022 21:57
@marten-seemann marten-seemann force-pushed the update-libp2p-v024 branch 3 times, most recently from ce10543 to c99adc3 Compare November 20, 2022 22:58
@BigLep BigLep requested a review from Jorropo November 21, 2022 06:08
@BigLep
Copy link
Contributor

BigLep commented Nov 21, 2022

@Jorropo : can you please work with @marten-seemann to get this landed? I want to make sure he has a dedicated point of contact so this doesn't stall. And if it makes sense to take something over, go for it. I leave it up to you guys to decide.

@marten-seemann
Copy link
Member Author

@Jorropo : can you please work with @marten-seemann to get this landed? I want to make sure he has a dedicated point of contact so this doesn't stall. And if it makes sense to take something over, go for it. I leave it up to you guys to decide.

  1. All of the PRs (except one, which I'll handle) concern ipfs repos. I'd appreciate if an IPFS steward could handle merging these, cut the respective releases and bubble those releases up to this PR.
  2. Other than that, I've seen the linter fail due to an fx deprecation warning. I assume this is because go-libp2p now also uses fx, and is probably bumping the fx version used for kubo (due to minimum version selection). This would need to be fixed inside the kubo code base, which again would best be handled by the kubo team.
  3. Getting this PR merged (obviously, we actually need to release v0.24.0 first, but that will happen early this week) is only part of the story. Some migrations are required around QUIC versions and WebTransport, I've opened migrations needed after updating go-libp2p v0.24.0 update #9410 last week explaining the steps needed.

Does that sound reasonable, @BigLep and @Jorropo?

@BigLep
Copy link
Contributor

BigLep commented Nov 21, 2022

Thanks for creating the overarching tracking issue @marten-seemann . I had missed it. The plan makes sense. I have it now on in our current 0.18 "iteration".

@marten-seemann
Copy link
Member Author

@Jorropo What are the plans for bubbling up the dependency updates?

@Jorropo
Copy link
Contributor

Jorropo commented Dec 1, 2022

@marten-seemann Ill take Care of it for 0.18 once 0.24 is released.
It will include turning webtransport on by default and the migration to add webtransport listens to people that listen on QUIC already.

@marten-seemann
Copy link
Member Author

It will include turning webtransport on by default and the migration to add webtransport listens to people that listen on QUIC already.

I'd suggest making this 2 separate PRs. One (this PR) that only updates go-libp2p to v0.24.0 (dealing only with breaking API / dependency changes), and then a follow-up to do the WebTransport-related changes.

@Jorropo
Copy link
Contributor

Jorropo commented Dec 1, 2022

@marten-seemann sounds like a good plan

@marten-seemann marten-seemann force-pushed the update-libp2p-v024 branch 2 times, most recently from 11d0a7e to b08e0ea Compare December 6, 2022 08:45
@marten-seemann
Copy link
Member Author

@Jorropo Updated the PR to the recently released v0.24.0.

@Jorropo Jorropo marked this pull request as ready for review December 7, 2022 13:54
@Jorropo
Copy link
Contributor

Jorropo commented Dec 7, 2022

When trying to use fx.Populate I get:

ERROR	core	core/builder.go:157	constructing the node: missing dependencies for function "reflect".makeFuncStub (/home/hugo/Documents/Scripts/go/src/reflect/asm_amd64.s:28): missing type: core.IpfsNode
Error: constructing the node (see log for full detail): missing dependencies for function "reflect".makeFuncStub (/home/hugo/Documents/Scripts/go/src/reflect/asm_amd64.s:28): missing type: core.IpfsNode

for some reason, this doesn't need to be investigated now, we can defer this to later, just mutting staticcheck's complain for now.

@marten-seemann
Copy link
Member Author

go-libp2p-kad-dht v0.19.0 was just released.

@BigLep BigLep mentioned this pull request Dec 7, 2022
@marten-seemann
Copy link
Member Author

image

I don't think this is ready for review yet. All the packages I linked in the original post still need to be released, and the new versions need to be bubbled up.

go.mod Outdated
github.com/libp2p/go-libp2p-loggables v0.1.0
github.com/libp2p/go-libp2p v0.24.0
github.com/libp2p/go-libp2p-http v0.4.0
github.com/libp2p/go-libp2p-kad-dht v0.18.1-0.20221120203245-43eacaff1f99
Copy link
Member

Choose a reason for hiding this comment

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

@marten-seemann since this is still wip, is it possible to include libp2p/go-libp2p-kad-dht#793?

Copy link
Member Author

Choose a reason for hiding this comment

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

We released v0.19.0 yesterday.

libp2p/go-libp2p-kad-dht#793 would require a new release, but this is orthogonal to this PR, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this could be a separate release and could be a separate PR.

I am flagging this because "required" list for Kubo 0.18 (#9417) includes #9389 which (iiuc) needs a new libp2p release with libp2p/go-libp2p-kad-dht#794 and libp2p/go-libp2p-kad-dht#793.

So either we need a patch release, or to descope the above from Kubo 0.18. (cc @BigLep)

Copy link
Member Author

Choose a reason for hiding this comment

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

I am flagging this because "required" list for Kubo 0.18 (#9417) includes #9389 which (iiuc) needs a new libp2p release with libp2p/go-libp2p-kad-dht#794 and libp2p/go-libp2p-kad-dht#793.

go-libp2p-kad-dht is not a dependency of go-libp2p, it's the other way around. So no new go-libp2p release required.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sweet! In that case is it ok for me to just merge libp2p/go-libp2p-kad-dht#794 and libp2p/go-libp2p-kad-dht#793 and make a new v0.20.0 release of go-libp2p-kad-dht?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lidel I've bumped go-libp2p-kad-dht to v0.19.0 in ipfs/go-namesys#33 as long as you don't break v0.19.0's api in v0.20.0 (which neither of the PR you linked do), I'm good.

Copy link
Member

Choose a reason for hiding this comment

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

Pinged some folks for review on go-libp2p-kad-dht PRs, but it should not block this PR.
We can bump in a follow-up one.

Copy link
Member

Choose a reason for hiding this comment

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

@Jorropo before you merge this PR, please incorporate go-libp2p-kad-dht update from #9491 🙏

@Jorropo
Copy link
Contributor

Jorropo commented Dec 9, 2022

I finally fixed it by doing this: https://github.com/ipfs/kubo/blob/ee11ef6d2ca0df494e4d709f8354109452185a47/internal/i-hate-what-I-am-doing-here.go it's horrible looking but we are good enough since this isn't actually compiled.

@Jorropo Jorropo marked this pull request as ready for review December 12, 2022 02:20
@Jorropo Jorropo enabled auto-merge (rebase) December 12, 2022 02:38
@Jorropo Jorropo merged commit 3a3a971 into master Dec 12, 2022
@Jorropo Jorropo deleted the update-libp2p-v024 branch December 12, 2022 02:49
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.

4 participants