Skip to content

Conversation

@MarcoPolo
Copy link
Collaborator

lidel, take a look and let me know your thoughts. This is targeting your branch.

This removes the goLogBridge interface check and having to modify the default slog logger. Instead you explicitly set the handler via gologshim.SetDefaultHandler.

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

I think this makes the code cleaner/clearer. Your call @lidel

lidel added a commit to ipfs/kubo that referenced this pull request Nov 3, 2025
wire gologshim to go-log's slog bridge via init() in cmd/ipfs/kubo/start.go

update go-libp2p to v0.44.1-0.20251029234611-789d14c6effe

see libp2p/go-libp2p#3419
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

This works for us also, pushed docs + some thoughts below:

On the trade-off here

Original #3418 required no changes in existing apps, just updating both go-log and go-libp2p to latest versions.

This PR requires every legacy application that used both go-log and go-libp2p to add this extra code:

  import (
      logging "github.com/ipfs/go-log/v2"
      "github.com/libp2p/go-libp2p/gologshim"
  )

  func init() {
      // After go-log's init() runs and installs slog bridge,
      // tell gologshim to use it      
      handler := slog.Default().Handler()      
      // Verify it's go-log's bridge
      type goLogBridge interface {
	      GoLogBridge()
      }
      if _, ok := handler.(goLogBridge); !ok {
	      panic("aborting startup: slog.Default() is not go-log's bridge, logs would be missing due to incorrect wiring")
      }
      // use the go-log bridge      
      gologshim.SetDefaultHandler(handler)
  }

If this is the price to pay to fix logs, we can do the above in Kubo, Rainbow and every other application Shipyard maintains, but everyone else will be missing logs unless they do the same.

Just a trade-off decision.

Next steps

@MarcoPolo lgtm, i've tested this PR in ipfs/kubo#11039 and it also works for us (with the above extra code).

Up to you if you prefer implicit or explicit.

  • If you prefer explicit, feel free to merge this PR into #3418, and then merge that (#3419+#3418).
  • If you are ok with implicit detection, this PR can be closed and only merge #3418

Either way, we would appreciate a release with a fix 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants