-
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
Changes from all commits
3e07cb5
7480d51
e344c1a
b72ed83
19d8f0c
526c7b9
26cbc0e
b8c90c1
92f03b4
e3385d5
de7a86e
04e6705
65143c9
abe62b4
3896f80
9a085b9
519bf89
2f4e915
8604789
5c22115
6036a40
383f947
8422e22
4765fb4
116ae79
555addd
1ba564f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,3 +113,9 @@ web-sys = { version = "0.3.52", features = ["Window"] } | |
|
||
[build-dependencies] | ||
re_build_tools.workspace = true | ||
|
||
# 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Issue filed: #3355 |
||
xshell = "0.2" |
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
containerThere 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 callingpip instal
here - we won't need to generate a newci_docker
all the time, but when we do these jobs will speed upThere 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:
Now I don't want to touch it in fear of what other horrors lie beyond. 🙂
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.