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

Fix unnecessary re_viewer dependency #6369

Merged
merged 2 commits into from
May 20, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented May 17, 2024

What

The mechanism for identifying Rerun clients was implemented in #6204. However, during the release I accidentally "fixed" the build of cargo build -p re_viewer --no-default-features twice:

  • The seemingly correct fix was to depend on re_sdk_comms with the server feature always enabled.
  • The definitely wrong thing that slipped through is to feature guard the section that needs the re_sdk_comms server errors with #[cfg(feature = "server")]. But re_viewer doesn't have a server feature, disabling this effectively. This was reverted in Don't log warnings when unknown clients connect over TCP #6368

It's up to users of re_viewer to allow the serve feature, making re_viewer agonistic of the presence of this error type in its smart channel. Therefore I now moved out the error type to re_sdk_comms lib.rs so that re_viewer doesn't have to depend on the re_sdk_comms's server feature.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

To run all checks from main, comment on the PR with @rerun-bot full-check.

@Wumpf Wumpf added 🪳 bug Something isn't working 📺 re_viewer affects re_viewer itself include in changelog labels May 17, 2024
@Wumpf
Copy link
Member Author

Wumpf commented May 17, 2024

this is really only a difference to #6368 if you somehow manage to depend on re_viewer without depending re_sdk_comms = { workspace = true, features = ["server"] }. Which as of now never happens as far as I can tell

@Wumpf
Copy link
Member Author

Wumpf commented May 17, 2024

corrections: the wasm blob itself shouldn't drag in re_sdk_comms with server enabled

@Wumpf Wumpf changed the title Fix log spam when a TCP client without the rerun protocol header tries to connect Fix log spam when a TCP client without the rerun protocol header tries to connect, fix unnecessary dependency May 17, 2024
@Wumpf Wumpf changed the title Fix log spam when a TCP client without the rerun protocol header tries to connect, fix unnecessary dependency Fix log spam when a TCP client without the rerun protocol header tries to connect & fix unnecessary dependency May 17, 2024
@Wumpf Wumpf changed the title Fix log spam when a TCP client without the rerun protocol header tries to connect & fix unnecessary dependency Fix unnecessary re_viewer dependency May 17, 2024
@Wumpf Wumpf added exclude from changelog PRs with this won't show up in CHANGELOG.md and removed include in changelog labels May 17, 2024
@emilk emilk merged commit 1f3004f into main May 20, 2024
36 of 38 checks passed
@emilk emilk deleted the andreas/fix-log-spam-on-unknown-client branch May 20, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working exclude from changelog PRs with this won't show up in CHANGELOG.md 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants