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.

Sign up for GitHub

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

Jump to bottom

re_sdk should not depend on ehttp #6090

Closed
emilk opened this issue Apr 24, 2024 · 1 comment · Fixed by #6210
Closed

re_sdk should not depend on ehttp #6090

emilk opened this issue Apr 24, 2024 · 1 comment · Fixed by #6210
Assignees
@emilk
Labels
😤 annoying Something in the UI / SDK is annoying to use 🌊 C++ API C or C++ logging API dependencies concerning crates, pip packages etc 🧑‍💻 dev experience developer experience (excluding CI)
Milestone
0.16

Comments

@emilk
Copy link
Member

emilk commented Apr 24, 2024 •
edited

Proplem

Pulling in ehttp mean pulling in rustls which adds dynamic linking to libssl on Linux, which has broken C++ builds for some of our users.

Cause

The PR #5330 added 70 crates of dependencies to re_sdk, and despite the warnings of our bot, no human noticed until today, almost TWO MONTHS later.

Screenshot 2024-04-24 at 08 15 09

rerun_c/Cargo.toml has re_sdk = { workspace = true, features = ["data_loaders"]} to support log_file_from_path.

The data_loaders feature pulls in re_data_source as a dependency.

rerun_c -> re_sdk -> re_data_source -> re_log_encoding -> ehttp -> ureq -> rustls

To support log_file_from_path we don't need all of re_data_source though. In particular, we only need the file loading part of it, NOT ehttp.

Related
@emilk emilk added 😤 annoying Something in the UI / SDK is annoying to use dependencies concerning crates, pip packages etc labels Apr 24, 2024
@emilk
Copy link
Member Author

emilk commented Apr 24, 2024

A proper developer dashboard would have caught this:

@emilk emilk added 🌊 C++ API C or C++ logging API 🧑‍💻 dev experience developer experience (excluding CI) labels Apr 24, 2024
@emilk emilk added this to the 0.16 milestone May 3, 2024
@emilk emilk self-assigned this May 3, 2024
@emilk emilk mentioned this issue May 3, 2024
Merged
5 tasks
emilk added a commit that referenced this issue May 3, 2024
@emilk
Fix: re_sdk no longer depends on rustls (#6210)
c667644 
Split out parts of `re_data_source` to a new crate `re_data_loader`.
This makes the dependency list of `re_sdk` much lighter.

* Closes #6090

`scripts/ci/count_dependencies.py -p re_sdk --all-features` goes from
204 -> 185 dependencies.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6210?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6210?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6210)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@emilk emilk

Labels
😤 annoying Something in the UI / SDK is annoying to use 🌊 C++ API C or C++ logging API dependencies concerning crates, pip packages etc 🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Milestone
0.16
Development

Successfully merging a pull request may close this issue.

Fix: re_sdk no longer depends on rustls rerun-io/rerun
1 participant
@emilk