-
Notifications
You must be signed in to change notification settings - Fork 337
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
Generate manifest for examples page in viewer #3332
Conversation
Cargo.lock
Outdated
@@ -1765,7 +1765,7 @@ checksum = "79386fdcec5e0fde91b1a6a5bcd89677d1f9304f7f986b154a1b9109038854d9" | |||
dependencies = [ | |||
"az", | |||
"bytemuck", | |||
"half 1.8.2", | |||
"half 2.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why it decided to do this, I only added serde_yaml
as a dev dependency. We're already on Rust 1.72, and fixed
has its half
version set as >=1.8,<3
, so it seems harmless.
panic!("{err}"); | ||
} | ||
} | ||
|
||
fn main() { | ||
re_build_tools::rebuild_if_crate_changed("re_viewer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will re-run this build.rs script if anything in re_viewer
or any of its dependancies changes. Isn't that a bit too often? I thought only the CI needed to run this at all? And it need to run it always,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build script has to run locally so that we can commit the examples_manifest.json
file when it changes. That way users can build the re_viewer
crate without being in the workspace. We need to trigger a run of the build script on any change to the examples
directory, but I don't know what the best way to detect if examples were added/changed/removed is. Always running it solves that, I think?
I tried profiling the build script, and maybe it won't turn into an issue. Before this PR, the script takes ~350ms to run. Calling write_examples_manifest
added ~500us for me. On CI it will add a bit more, because it also has to run git
a few times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed where I was.
We need to trigger a run of the build script on any change to the examples directory, but I don't know what the best way to detect if examples were added/changed/removed is
For changed/removed, use re_build_tools::rerun_if_changed
for each file you read. It does println!("cargo:rerun-if-changed=…
internally.
Detecting added files… I don't know how to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always running it solves that, I think?
If you want to force the build.rs
file to always run (which you may want to on CI), there is an ugly trick: println!("cargo:rerun-if-changed=path/to/file/that/doesnt/exist");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should already always run on CI, because all the crates are built from scratch
@@ -266,6 +266,7 @@ jobs: | |||
- name: Install dependencies | |||
run: | | |||
pip install gitignore_parser python-frontmatter | |||
pip install -r ./scripts/ci/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither of these should be needed if we would run the job on our ci_docker
container
container:
image: rerunio/ci_docker:0.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the docker container is a hack we should use as little as possible, because someone always has to remember to update it any time we add new dependencies. The job here takes 1 minute to run, and it runs in parallel to all the other checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take your point, though the container is not a hack as much as an optimization and a cost saving device (CI minutes cost money). I would also rather not have it, but 🤷
We could get the best of both worlds though, by starting with the ci_docker
container and then still calling pip instal
here - we won't need to generate a new ci_docker
all the time, but when we do these jobs will speed up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to try it to see how much of a speedup it would be, because I suspect it might actually be a slowdown, but:
python3: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by python3)
Now I don't want to touch it in fear of what other horrors lie beyond. 🙂
though the container is not a hack as much as an optimization and a cost saving device
I understand that, but I think there are better ways to accomplish the same thing. The docker image is basically our own weird cache that we have to manually keep up to date. Doing proper caching in every job would likely have a much bigger impact, but it is a lot more short-term effort than just adding another tool into the image, pushing a new tag, and updating the CI to use it.
# External | ||
serde = { version = "1", features = ["derive"] } | ||
serde_json = "1" | ||
serde_yaml = "=0.9.21" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, afaict this is to parse the markdown frontmatter, correct? Could we be using toml
in there instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'd also have to update everything else that reads the frontmatter. Currently that's at least landing and build_demo_app.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do 🙏 , but can be saved for another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue filed: #3355
panic!("{err}"); | ||
} | ||
} | ||
|
||
fn main() { | ||
re_build_tools::rebuild_if_crate_changed("re_viewer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed where I was.
We need to trigger a run of the build script on any change to the examples directory, but I don't know what the best way to detect if examples were added/changed/removed is
For changed/removed, use re_build_tools::rerun_if_changed
for each file you read. It does println!("cargo:rerun-if-changed=…
internally.
Detecting added files… I don't know how to do.
panic!("{err}"); | ||
} | ||
} | ||
|
||
fn main() { | ||
re_build_tools::rebuild_if_crate_changed("re_viewer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always running it solves that, I think?
If you want to force the build.rs
file to always run (which you may want to on CI), there is an ugly trick: println!("cargo:rerun-if-changed=path/to/file/that/doesnt/exist");
@@ -266,6 +266,7 @@ jobs: | |||
- name: Install dependencies | |||
run: | | |||
pip install gitignore_parser python-frontmatter | |||
pip install -r ./scripts/ci/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take your point, though the container is not a hack as much as an optimization and a cost saving device (CI minutes cost money). I would also rather not have it, but 🤷
We could get the best of both worlds though, by starting with the ci_docker
container and then still calling pip instal
here - we won't need to generate a new ci_docker
all the time, but when we do these jobs will speed up
# External | ||
serde = { version = "1", features = ["derive"] } | ||
serde_json = "1" | ||
serde_yaml = "=0.9.21" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue filed: #3355
crates/re_viewer/build.rs
Outdated
let thumbnail_dimensions = get!(example, thumbnail_dimensions); | ||
|
||
Ok(Self { | ||
title: get!(example, title), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a macro to unwrap the title: Option<String>
instead of using title: String
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think originally I just wanted the parsing from YAML to not fail early, so that I could check for the presence of build_args
. I changed it to #[serde(default)]
all the fields instead, and removed the macro.
- `Frontmatter` does not need `Option` fields - `parse_release_version` does not need to format the version
What
Part of #3212
thumbnail
havethumbnail_dimensions
tooscripts/ci/thumbnails.py
script for checking/updatingthumbnail_dimensions
build.rs
tore_viewer
re_viewer/data/examples_manifest.json
up to dateversion/nightly
onmain
/local buildsversion/x.y.z
on release branchescommit/{short_hash}
on PRsChecklist