Update imgui version to 1.92.4 and add more backends #6460
Conversation
|
Hello @phaedon, modules you maintain (imgui) have been updated in this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates imgui to version 1.92.4 and adds support for SDL2 backends. The changes look good overall, but there are a few critical issues to address. The presubmit configuration is missing a necessary dependency for the new SDL2 backend, which will cause CI to fail. Additionally, the implementation for the SDL2 backends relies on hardcoded paths to system-installed libraries, which harms reproducibility and is strongly discouraged in the BCR. I've left specific comments with suggestions to fix these issues.
| # X11 requires libx11-dev and a few other system packages, all in xorg-dev. | ||
| # See https://www.glfw.org/docs/latest/compile.html#compile_deps | ||
| - sudo apt-get update | ||
| - sudo apt-get install xorg-dev libgl1-mesa-dev -y |
There was a problem hiding this comment.
The presubmit configuration for Linux is missing the development package for SDL2. Since you are adding an SDL2 backend and example, libsdl2-dev needs to be installed for the presubmit build to pass. Without it, the build of @imgui//... will fail when it tries to compile the SDL2 example.
- sudo apt-get install xorg-dev libgl1-mesa-dev libsdl2-dev -y| cc_library( | ||
| name = "platform-sdl2", | ||
| srcs = ["imgui_impl_sdl2.cpp"], | ||
| hdrs = ["imgui_impl_sdl2.h"], | ||
| copts = ["-I/usr/include/SDL2"], | ||
| include_prefix = "imgui", | ||
| includes = ["."], | ||
| linkopts = ["-lSDL2"], | ||
| deps = ["//:imgui"], | ||
| ) |
There was a problem hiding this comment.
The use of hardcoded paths like copts = ["-I/usr/include/SDL2"] and linkopts = ["-lSDL2"] for system-provided libraries is strongly discouraged. This approach breaks build hermeticity and reproducibility, which are key goals of the Bazel Central Registry 1. It also goes against the principle of building from source whenever possible 2. While I understand your note about not wanting to fight SDL's build system, this build will fail on any system where SDL2 is not installed in this exact location. The same issue exists in the renderer-sdl2 target (lines 164-173). For a public module, a more robust solution is required. Please consider adding SDL2 as a proper BCR module or, failing that, using a repository rule that can discover the SDL2 installation (e.g., via pkg-config) to avoid hardcoding paths.
Style Guide References
Footnotes
| cc_binary( | ||
| name = "example_sdl2_sdlrenderer2", | ||
| srcs = ["main.cpp"], | ||
| copts = ["-I/usr/include/SDL2"], |
There was a problem hiding this comment.
This hardcoded include path copts = ["-I/usr/include/SDL2"] makes the example non-portable and suffers from the same reproducibility and hermeticity issues as the backend targets 1. This should be resolved in conjunction with the backend library definitions.
Style Guide References
Footnotes
-
BCR PRs must ensure that modules are reproducible for downstream users. ↩
|
@bazel-io skip_check unstable_url |
bazel-io
added
the
skip-url-stability-check
| @@ -0,0 +1 @@ | |||
| ../MODULE.bazel No newline at end of file | |||
There was a problem hiding this comment.
Symlinks are not allowed anymore - replace this with a copy of MODULE.bazel
3 participants
Firstly, this updates imgui to the latest release (v1.92.4).
Secondly, it adds the sdl2 platform and sdlrenderer2 renderer. These are a little unusual in that SDL is not easily statically linked and I don't really have any interest in fighting against that to make SDL build with bazel. As such, it links the system-installed versions of SDL when building. I understand this is not the usual way of using bazel.