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 web viewer feature flags #8295

Merged
merged 9 commits into from
Dec 3, 2024
Merged

Fix web viewer feature flags #8295

merged 9 commits into from
Dec 3, 2024

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Dec 3, 2024

Related

What

  • build-web-viewer now propagates --no-default-features, and doesn't include it by default
  • Made map_view a default feature in re_viewer

@jprochazk jprochazk added 🕸️ web regarding running the viewer in a browser 📺 re_viewer affects re_viewer itself include in changelog labels Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link
b7e8d58 https://rerun.io/viewer/pr/8295

Note: This comment is updated whenever you push a commit.

Copy link

github-actions bot commented Dec 3, 2024

Latest documentation preview deployed successfully.

Result Commit Link
b7e8d58 https://landing-iiobifqen-rerun.vercel.app/docs

Note: This comment is updated whenever you push a commit.

@@ -31,7 +31,8 @@ function buildWebViewer(mode) {
"cargo run -p re_dev_tools -- build-web-viewer",
modeFlags,
"--target no-modules-base",
"--features grpc",
"--no-default-features",
"--features grpc,map_view",
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we have a release = ["grpc", "map_view"] feature in re_viewer instead. It feels like a more discoverable location for controlling what goes into the release.

Suggested change
"--features grpc,map_view",
"--features release",

Copy link
Member Author

@jprochazk jprochazk Dec 3, 2024

Choose a reason for hiding this comment

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

At this point I just made map_view a default feature. I don't agree with the release feature for web viewer specifically, it makes sense for rerun-cli because there it makes it possible for it to be compiled without nasm, and with -F release in case you do want nasm.

Copy link
Member Author

@jprochazk jprochazk Dec 3, 2024

Choose a reason for hiding this comment

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

The big problem here imho was build_web_viewer silently passing --no-default-features to the re_viewer build

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

@jprochazk
Copy link
Member Author

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Nice!

Btw, I wonder if we shouldn't make a re_web_viewer crate that just wraps re_viewer. re_viewer is weird in that it is both a library, and web app. Food for thought

@@ -31,7 +31,8 @@ function buildWebViewer(mode) {
"cargo run -p re_dev_tools -- build-web-viewer",
modeFlags,
"--target no-modules-base",
"--features grpc",
"--no-default-features",
"--features grpc,map_view", // no `analytics`
Copy link
Member

Choose a reason for hiding this comment

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

why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is for the NPM package, we don't want users' applications which embed the web viewer this way to send analytics to us. they will only do that if they embed the web viewer as an iframe

Comment on lines +178 to +200
rerun-build-web = "rustup target add wasm32-unknown-unknown && cargo run -p re_dev_tools -- build-web-viewer --no-default-features --features analytics,grpc,map_view --debug"

# Compile the web-viewer wasm and the cli.
#
# This installs the `wasm32-unknown-unknown` rust target if it's not already installed.
# (this looks heavy but takes typically below 0.1s!)
rerun-build-web-cli = "rustup target add wasm32-unknown-unknown && cargo run -p re_dev_tools -- build-web-viewer --features analytics,grpc --debug && cargo build --package rerun-cli --no-default-features --features web_viewer"
rerun-build-web-cli = "rustup target add wasm32-unknown-unknown && cargo run -p re_dev_tools -- build-web-viewer --no-default-features --features analytics,grpc,map_view --debug && cargo build --package rerun-cli --no-default-features --features web_viewer"

# Compile and run the web-viewer in release mode via rerun-cli.
#
# You can also give an argument for what to view (e.g. an .rrd file).
#
# This installs the `wasm32-unknown-unknown` rust target if it's not already installed.
# (this looks heavy but takes typically below 0.1s!)
rerun-web-release = { cmd = "cargo run --package rerun-cli --no-default-features --features web_viewer --release -- --web-viewer", depends_on = [
rerun-web-release = { cmd = "cargo run --package rerun-cli --no-default-features --features web_viewer,map_view,grpc --release -- --web-viewer", depends_on = [
"rerun-build-web-release",
] }

# Compile the web-viewer wasm in release mode.
#
# This installs the `wasm32-unknown-unknown` rust target if it's not already installed.
# (this looks heavy but takes typically below 0.1s!)
rerun-build-web-release = "rustup target add wasm32-unknown-unknown && cargo run -p re_dev_tools -- build-web-viewer --features analytics,grpc --release -g"
rerun-build-web-release = "rustup target add wasm32-unknown-unknown && cargo run -p re_dev_tools -- build-web-viewer --no-default-features --features analytics,grpc,map_view --release -g"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't easier to keep the default features and just add on grpc?

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 wanted to be more explicit here, because you're not going to type the long command

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Nice!

Btw, I wonder if we shouldn't make a re_web_viewer crate that just wraps re_viewer. re_viewer is weird in that it is both a library, and web app. Food for thought

@jprochazk
Copy link
Member Author

jprochazk commented Dec 3, 2024

Btw, I wonder if we shouldn't make a re_web_viewer crate that just wraps re_viewer. re_viewer is weird in that it is both a library, and web app. Food for thought

@jprochazk jprochazk merged commit 312f91c into main Dec 3, 2024
37 checks passed
@jprochazk jprochazk deleted the jan/fix-web-viewer-features branch December 3, 2024 14:02
jprochazk added a commit that referenced this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 📺 re_viewer affects re_viewer itself 🕸️ web regarding running the viewer in a browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

map-view feature not enabled in web-viewer
2 participants