Add gmsh installation for docs build#148
Conversation
Install gmsh and its dependencies as a prerequisite for docs-clean, ensuring the docs build environment has the required meshing tools.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a dedicated Makefile target to install gmsh and its system dependencies, and wires it into the docs-clean/docs build flow so that documentation builds have the required meshing tools available. Flow diagram for updated docs build Makefile targetsflowchart TD
A[make docs] --> B[docs-clean target]
B --> C[gmsh target]
C --> D[apt-get update]
D --> E[apt-get install python3-gmsh]
D --> F[apt-get install gmsh]
D --> G[apt-get install libglu1-mesa]
D --> H[apt-get install libxi-dev]
D --> I[apt-get install libxmu-dev]
D --> J[apt-get install libglu1-mesa-dev]
B --> K[rm -rf docs/_build]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Having the
gmshtarget runsudo apt-getassumes a Debian-based system with sudo available; consider moving system package installation to your CI/bootstrap scripts or a clearly separated setup target so the Makefile remains usable in non-privileged and non-Debian environments. - Making
docs-cleandepend ongmshcauses every docs clean to trigger package installation; consider depending ongmshfromdocs(or a dedicateddocs-setuptarget) instead so that cleaning the build directory doesn't imply system-level changes. - Running
apt-get updateinside thegmshtarget each time can be slow and flaky; consider either removing it in favor of CI-level cache management or guarding the target so it only installs when the packages are missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Having the `gmsh` target run `sudo apt-get` assumes a Debian-based system with sudo available; consider moving system package installation to your CI/bootstrap scripts or a clearly separated setup target so the Makefile remains usable in non-privileged and non-Debian environments.
- Making `docs-clean` depend on `gmsh` causes every docs clean to trigger package installation; consider depending on `gmsh` from `docs` (or a dedicated `docs-setup` target) instead so that cleaning the build directory doesn't imply system-level changes.
- Running `apt-get update` inside the `gmsh` target each time can be slow and flaky; consider either removing it in favor of CI-level cache management or guarding the target so it only installs when the packages are missing.
## Individual Comments
### Comment 1
<location path="Makefile" line_range="35-37" />
<code_context>
python -m build
-docs-clean:
+gmsh:
+ sudo apt-get update
+ sudo apt-get install -y python3-gmsh gmsh libglu1-mesa libxi-dev libxmu-dev libglu1-mesa-dev
+
+docs-clean: gmsh
</code_context>
<issue_to_address>
**issue:** Running `sudo apt-get` inside a make target can break non-root or CI usage.
This target assumes `sudo` is available and that the system uses `apt-get`, which will fail or be disallowed in many CI/container/non-root environments and can cause side effects when running `make`. Please either treat these as documented manual prerequisites, move them to a separate setup script, or gate this target behind an explicit opt-in flag (e.g. `ENABLE_SYSTEM_INSTALL=1`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| gmsh: | ||
| sudo apt-get update | ||
| sudo apt-get install -y python3-gmsh gmsh libglu1-mesa libxi-dev libxmu-dev libglu1-mesa-dev |
There was a problem hiding this comment.
issue: Running sudo apt-get inside a make target can break non-root or CI usage.
This target assumes sudo is available and that the system uses apt-get, which will fail or be disallowed in many CI/container/non-root environments and can cause side effects when running make. Please either treat these as documented manual prerequisites, move them to a separate setup script, or gate this target behind an explicit opt-in flag (e.g. ENABLE_SYSTEM_INSTALL=1).
Re-run check-template-drift pre-commit hook to rewrite `secrets: inherit` to explicit per-workflow secret forwarding, matching the upstream pdk-ci-workflow template. uv.lock regenerated to reflect the current gdsfactory~=9.40.1 pin in pyproject.toml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The palace_demo_microstrip notebook calls sim.plot_mesh() which invokes pyvista.Plotter(off_screen=True); without OSMesa or EGL system libraries VTK fails to initialize a GL context and the kernel segfaults, aborting the Sphinx build. Adding libosmesa6 and libegl1 to the gmsh apt-get line provides the software GL stack required for headless rendering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
gmshMakefile target that installs gmsh and its dependencies (libglu1-mesa, libxi-dev, libxmu-dev)docs-cleandepend ongmshso the docs build environment has the required meshing toolsTest plan
make docssucceeds in CI with gmsh availableSummary by Sourcery
Add a Makefile target to install gmsh and its system dependencies and ensure the docs-clean step prepares a docs build environment with gmsh available.
Build: