CI: Use static MRBind binaries for generating C bindings#6313
Conversation
Fedr
left a comment
There was a problem hiding this comment.
Nice speedup — the C-bindings job drops to ~1m35s, and the mechanics line up cleanly: the zip's mrbind/ → thirdparty/mrbind/build/ matches MRBIND_EXE := $(MRBIND_SOURCE)/build/mrbind, and CLANG_RESOURCE_DIR=.../build/resource-dir matches the bundled resource dir. Dropping the deps/cppdecl submodule checkout is correct now that we don't compile mrbind. A few things below.
Should-fix
1. Leftover debug output. These two lines in "Download and unpack MRBind" don't do anything functional (from the WIP: Debug output commit) — the first just prints the release list, the second prints nothing since v0.0.1 isn't a draft:
gh release list --repo MeshInspector/mrbind
gh api repos/MeshInspector/mrbind/releases --jq '.[] | select(.draft) | .assets[].url'Probably want to drop both before merge.
2. contents: write looks over-privileged for what's actually downloaded. v0.0.1 is a published prerelease (draft:false, prerelease:true), and prereleases download fine with contents: read. write is only needed for genuine draft releases. As a reusable workflow this also forces every caller to grant write (today only build-test-distribute.yml calls it, which happens to have top-level contents: write). If releases stay as prereleases, could this go back to contents: read? If draft is the intended end-state, maybe worth a comment saying so.
Worth discussing
3. Version skew between the pinned binary and the submodule. The binary is now pinned via mrbind_version.txt, but generate.mk still consumes mrbind sources from the checked-out submodule — -I $(MRBIND_SOURCE)/include and the mrbind-pybind11 headers. Nothing ties the release binary's build commit to the submodule SHA the superproject pins, whereas build-mrbind keyed its cache on that SHA. If the submodule gets bumped without bumping mrbind_version.txt (or the release is rebuilt from a different commit), the generator and the runtime headers can drift apart silently. Might be worth a documented sync step.
Minor / robustness
4. alias gh=... only works because this step runs under sh. dash expands aliases in non-interactive scripts; bash does not — and the next step explicitly sets shell: bash. If this step ever gets "normalized" to bash, gh silently falls back to system gh or fails. A plain variable (GH=gh_2.95.0_linux_arm64/bin/gh, then "$GH" ...) or prepending the bin dir to PATH would be more robust and shell-agnostic.
5. curl -sL ... | tar xz has no pipefail under sh -e. A failed download won't fail at the pipe — only the later unzip catches it, with a less obvious error.
6. Pinned gh v2.95.0 is re-downloaded every run from a hardcoded URL with no checksum. A one-line comment on why gh has to be fetched (container lacks it?) and the pin rationale would help; caching it would save the download.
7. Nits. Trailing whitespace after contents: write. mv mrbind thirdparty/mrbind/build assumes build/ doesn't already exist (fine for fresh CI checkout). And the PR body is empty — a couple of lines noting this is a first step while other jobs still build from source would help future readers.
No description provided.