refactor: move ICU env vars from build script to nix shell config - #1325
Conversation
Instead of running nix-build inline during each Go build, set PKG_CONFIG_PATH, CGO_CFLAGS, and CGO_LDFLAGS for icu.dev in fish/bash/zsh shell init and the make-updater systemd service. Also includes build robustness fixes: - Fall through to Cargo.toml when Makefile has no build target - Auto-trust mise.toml before installing tools - Init git submodules before building if .gitmodules exists
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe PR adds ICU (International Components for Unicode) library support for CGo-based builds across multiple shell configurations (bash, fish, zsh) and a systemd service by configuring pkg-config paths and compiler/linker flags. It also enhances the build script with mise trust verification, git submodule initialization, and improved Makefile build-target detection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mesa DescriptionTL;DRRefactor: move ICU environment variables from build script to Nix shell configuration. What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 5ea3ff9...42f8043
Analysis
• Darwin launchd service is missing ICU environment variables (PKG_CONFIG_PATH, CGO_CFLAGS, CGO_LDFLAGS) that were added to the Linux systemd service. This creates a critical platform inconsistency where automated updates via macOS launchd will fail to build Go projects with ICU dependencies, while interactive shells work fine.
• Development shell (devenv.nix) lacks ICU environment variables present in production shell configs, creating an inconsistency where developers may not catch build failures that only manifest in automated environments.
• The refactoring delegates platform-specific package path resolution to Nix configs (good), but incomplete application across all execution contexts (launchd, devenv) undermines the architectural goal of centralized, consistent configuration management.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 0 comments | Edit Agent Settings • Read Docs
![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 adds ICU and CGo environment variables to shell configurations and the make-updater service to support CGo-based builds. Additionally, the update-local-binaries script is updated to trust mise configurations, initialize git submodules, and handle Makefile build targets more flexibly by falling back to other build systems when necessary. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Refactors ICU-related environment setup by moving ICU CGo/pkg-config flags out of the build script and into nix shell/service configuration, while also improving build robustness for repos using mise, git submodules, and varying Makefile targets.
Changes:
- Add ICU
PKG_CONFIG_PATH,CGO_CFLAGS, andCGO_LDFLAGSto fish/bash/zsh shell init (Linux) and to themake-updatersystemd service. - Remove/avoid implicit ICU build logic in
update-local-binaries.sh, and improve Makefile target fallthrough behavior. - Add robustness steps:
mise trustand git submodule init.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/update-local-binaries.sh | Adds mise trust + submodule init and adjusts Makefile build/fallthrough logic; notes ICU env vars come from shell init. |
| home-manager/services/make-updater/default.nix | Sets ICU/OpenSSL-related PKG_CONFIG_PATH and CGo flags for the systemd service environment. |
| home-manager/programs/zsh/default.nix | Exports ICU CGo/pkg-config env vars for Linux shells. |
| home-manager/programs/fish/default.nix | Exports ICU CGo/pkg-config env vars for Linux fish shells. |
| home-manager/programs/bash/default.nix | Exports ICU CGo/pkg-config env vars for Linux bash shells (in both relevant init blocks). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Install tools via mise if mise.toml is present | ||
| if [ -f "$build_dir/mise.toml" ] && command -v mise >/dev/null 2>&1; then | ||
| log_step " Installing tools via mise..." | ||
| (cd "$build_dir" && mise trust 2>&1) || true |
There was a problem hiding this comment.
mise trust persists a trust decision for the directory (and potentially its tool config) into the user’s mise trust store. Running it automatically inside an update/build script can unintentionally trust configs from repos the user didn’t explicitly approve. Prefer avoiding persistent trust changes from automation (e.g., require an explicit opt-in flag/env var before running mise trust, or use a scoped/non-persistent mechanism if mise supports it), so the script can build without mutating global trust state.
| (cd "$build_dir" && mise trust 2>&1) || true | |
| if [ "${MISE_AUTO_TRUST:-}" = "1" ]; then | |
| (cd "$build_dir" && mise trust 2>&1) || true | |
| else | |
| log_warn " Skipping 'mise trust' because MISE_AUTO_TRUST is not set to 1; run 'mise trust' manually if needed." | |
| fi |
| # Initialize git submodules if .gitmodules exists | ||
| if [ -f "$repo_dir/.gitmodules" ]; then | ||
| log_step " Initializing submodules..." | ||
| (cd "$repo_dir" && git submodule update --init --recursive 2>&1) || true |
There was a problem hiding this comment.
Submodule initialization is performed in repo_dir, but builds are executed from build_dir throughout this function. If build_dir is a separate checkout/copy (as the surrounding code implies), submodules in the build tree won’t be initialized and builds can fail. Initialize submodules in the same directory you build from (and check .gitmodules in that directory) to keep the operation consistent and effective.
| # Initialize git submodules if .gitmodules exists | |
| if [ -f "$repo_dir/.gitmodules" ]; then | |
| log_step " Initializing submodules..." | |
| (cd "$repo_dir" && git submodule update --init --recursive 2>&1) || true | |
| # Initialize git submodules if .gitmodules exists in the build directory | |
| if [ -f "$build_dir/.gitmodules" ]; then | |
| log_step " Initializing submodules..." | |
| (cd "$build_dir" && git submodule update --init --recursive 2>&1) || true |
| # If Makefile has no build target, fall through to other build systems | ||
| if (cd "$build_dir" && $make_cmd -n build >/dev/null 2>&1); then | ||
| if (cd "$build_dir" && $make_cmd build 2>&1); then | ||
| return 0 | ||
| else | ||
| return 1 | ||
| fi | ||
| else | ||
| return 1 | ||
| log_warn " Makefile has no build target, trying other build systems..." | ||
| fi |
There was a problem hiding this comment.
Using make -n build as a proxy for 'target exists' can misclassify real Makefile errors (e.g., missing included files, syntax errors, required env) as 'no build target', causing the script to silently fall through to other build systems and hide the actual failure. A more reliable approach is to only fall through when the failure is specifically 'No rule to make target …', and otherwise treat it as a genuine Makefile error (return non-zero and surface the output).
Summary
PKG_CONFIG_PATH,CGO_CFLAGS,CGO_LDFLAGSforicu.devto fish/bash/zsh shell init (Linux block) — same pattern as existingopenssl.devsetupmake-updatersystemd serviceEnvironmentnix-buildICU logic fromupdate-local-binaries.shTest plan
make buildsucceedsmake shell-testpasses (no new failures)make formatclean (0 changed)🤖 Generated with Claude Code
Summary by cubic
Move ICU CGo env vars from the build script into Nix shell init and the
make-updaterservice to standardize builds. Removes inlinenix-buildand makes Go/CGo builds more reliable.PKG_CONFIG_PATH,CGO_CFLAGS,CGO_LDFLAGSin Bash/Zsh/Fish (Linux), matching the existing OpenSSL setup.make-updatersystemd unitEnvironment.nix-buildlogic fromupdate-local-binaries.sh.mise trustbeforemise install..gitmodulesis present.make buildtarget and try Cargo/Go build paths.Written for commit 42f8043. Summary will update on new commits.