Simplify GHC packaging by extracting bootstrap and build tools#203
Open
0chroma wants to merge 3 commits into
Open
Simplify GHC packaging by extracting bootstrap and build tools#2030chroma wants to merge 3 commits into
0chroma wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extracts alex, happy, and ghc-bootstrap as local build packages with Nickel specs and Bash build scripts, integrates them into the ChangesGHC Toolchain Refactor
Sequence Diagram(s)sequenceDiagram
participant GHC_build_sh as packages/ghc/build.sh
participant GHC_bootstrap_sh as packages/ghc-bootstrap/build.sh
participant Alex_build_sh as packages/alex/build.sh
participant Happy_build_sh as packages/happy/build.sh
GHC_build_sh->>GHC_bootstrap_sh: extract/install prebuilt GHC -> install into $OUTPUT_DIR/usr
GHC_build_sh->>Alex_build_sh: invoke alex build (version via build_args)
GHC_build_sh->>Happy_build_sh: invoke happy build (version via build_args)
GHC_build_sh->>GHC_bootstrap_sh: copy/fix libtinfo and patch settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/alex/build.sh`:
- Around line 1-12: The build script must change into the extracted, versioned
source directory before building and reference the version variable; update the
top of the script to cd into the directory named by $MINIMAL_ARG_VERSION (e.g.,
cd "${MINIMAL_ARG_VERSION}" or cd "$MINIMAL_ARG_VERSION") before invoking ghc
and the ./Setup commands (Setup.hs, ghc --make Setup.hs, ./Setup
configure/build/copy), ensuring $OUTPUT_DIR is still used for copy destdir.
- Around line 4-10: The build script always compiles "Setup.hs" even when only
"Setup.lhs" exists; update the logic to select the existing setup file and
compile that instead (e.g., determine a SETUP_FILE variable that is "Setup.hs"
if present, otherwise "Setup.lhs", and use that variable in the ghc --make and
./Setup commands). Ensure references to Setup.hs in the compile and run steps
are replaced by this selected setup filename so the path where only Setup.lhs is
provided succeeds.
In `@packages/ghc-bootstrap/build.sh`:
- Around line 27-35: The script hardcodes /usr/lib/libncursesw.so.6 when
creating the libtinfo.so.6 shim; update it to locate the real ncurses library
(search /usr/lib* and /usr/lib64 or use ldconfig -p/grep or gcc
-print-file-name) into a variable (e.g., NCURSES_PATH) and use that variable
when copying to "$OUTPUT_DIR/usr/lib/libtinfo.so.6" and to
"$GHC_LIB_DIR/libtinfo.so.6"; if no library is found, emit a clear error and
exit non‑zero. Ensure you reference OUTPUT_DIR, GHC_LIB_DIR and
MINIMAL_ARG_VERSION and replace the two cp -p /usr/lib/libncursesw.so.6
occurrences with cp -p "$NCURSES_PATH" ... .
In `@packages/ghc/build.sh`:
- Around line 42-46: The current export of LD_LIBRARY_PATH can produce a
trailing colon when LD_LIBRARY_PATH is unset, causing ld.so to treat it as the
current directory; update the export that sets LD_LIBRARY_PATH (the line using
PSEUDO_LIBS and export LD_LIBRARY_PATH) so it only inserts a ':' between
PSEUDO_LIBS and the existing LD_LIBRARY_PATH when LD_LIBRARY_PATH is non-empty
(i.e., append the old value conditionally via parameter expansion or an
if-check), ensuring no trailing empty entry is added.
In `@packages/happy/build.sh`:
- Around line 1-12: The build script doesn't cd into the extracted versioned
source directory nor reference the MINIMAL_ARG_VERSION variable; update
packages/happy/build.sh to first change directory into the extracted source
(e.g. cd "happy-$MINIMAL_ARG_VERSION" or whichever directory name pattern is
used) before running ghc and ./Setup commands, and replace any hardcoded paths
(if present) with references that use $MINIMAL_ARG_VERSION and keep OUTPUT_DIR
usage for copy/destdir; ensure the cd happens before checking/creating Setup.hs
and before running ghc and ./Setup configure/build/copy.
- Around line 4-10: The build script assumes Setup.hs is present when running
GHC; change the compile step to pick the existing setup file dynamically so
Setup.lhs is handled too: after the block that creates Setup.hs only when
neither file exists, invoke GHC with the actual setup filename (use Setup.lhs if
it exists, otherwise Setup.hs) for the "ghc --make" step, then continue to run
the generated ./Setup configure --prefix=/usr; update the reference to the
compiled filename in the subsequent commands to match the chosen source
(Setup.lhs or Setup.hs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbb8e42f-b3de-44f1-b199-9d03f99c6dec
📒 Files selected for processing (8)
packages/alex/build.nclpackages/alex/build.shpackages/ghc-bootstrap/build.nclpackages/ghc-bootstrap/build.shpackages/ghc/build.nclpackages/ghc/build.shpackages/happy/build.nclpackages/happy/build.sh
… and safe LD_LIBRARY_PATH
Contributor
Author
|
/build |
Replace glob pattern usr/bin/* with explicit binary enumeration for the happy and alex packages to satisfy the enumerate bins checker.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ghc-bootstrap/build.sh`:
- Around line 58-68: The ldconfig pipeline in the NCURSES_PATH fallback can
cause the script to abort under set -euo pipefail; modify the assignment to
LDCONFIG_PATH so pipeline failures don't propagate (e.g. replace the pipeline
with a single awk that matches and exits: ldconfig -p | awk
'/libncursesw.so.6|libncurses.so.6/ {print $NF; exit}' or append "|| true" to
the command substitution), ensuring LDCONFIG_PATH becomes empty on no-match and
lets the subsequent if [ -f "$LDCONFIG_PATH" ] check run; update the block that
sets NCURSES_PATH from LDCONFIG_PATH accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1f448cc-0e6e-401d-ae99-83d1d19f8c27
📒 Files selected for processing (7)
packages/alex/build.nclpackages/alex/build.shpackages/ghc-bootstrap/build.nclpackages/ghc-bootstrap/build.shpackages/ghc/build.shpackages/happy/build.nclpackages/happy/build.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/alex/build.ncl
- packages/ghc-bootstrap/build.ncl
- packages/alex/build.sh
Comment on lines
+58
to
+68
| if [ -z "$NCURSES_PATH" ]; then | ||
| if command -v ldconfig >/dev/null 2>&1; then | ||
| for libname in libncursesw.so.6 libncurses.so.6; do | ||
| LDCONFIG_PATH="$(ldconfig -p | grep "$libname" | head -n1 | awk '{print $NF}')" | ||
| if [ -f "$LDCONFIG_PATH" ]; then | ||
| NCURSES_PATH="$LDCONFIG_PATH" | ||
| break | ||
| fi | ||
| done | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
ldconfig fallback can abort the script under set -euo pipefail, bypassing the clean error path.
The assignment at Line 61 runs a pipeline inside command substitution. Two cases break under set -euo pipefail:
- No match:
grepexits 1,pipefailpropagates it, andset -eaborts the script here instead of falling through to the intended error message at Lines 70‑73. - Match present:
head -n1closes the pipe early, sogrepcan receiveSIGPIPE(exit 141), again trippingpipefail/set -eeven though a path was found.
This makes the last-resort fallback unreliable. Guard the pipeline so a non-zero status yields an empty value and lets control reach the explicit check.
🛡️ Proposed fix
for libname in libncursesw.so.6 libncurses.so.6; do
- LDCONFIG_PATH="$(ldconfig -p | grep "$libname" | head -n1 | awk '{print $NF}')"
+ LDCONFIG_PATH="$(ldconfig -p 2>/dev/null | grep -F "$libname" | head -n1 | awk '{print $NF}' || true)"
if [ -f "$LDCONFIG_PATH" ]; then
NCURSES_PATH="$LDCONFIG_PATH"
break
fi
done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ghc-bootstrap/build.sh` around lines 58 - 68, The ldconfig pipeline
in the NCURSES_PATH fallback can cause the script to abort under set -euo
pipefail; modify the assignment to LDCONFIG_PATH so pipeline failures don't
propagate (e.g. replace the pipeline with a single awk that matches and exits:
ldconfig -p | awk '/libncursesw.so.6|libncurses.so.6/ {print $NF; exit}' or
append "|| true" to the command substitution), ensuring LDCONFIG_PATH becomes
empty on no-match and lets the subsequent if [ -f "$LDCONFIG_PATH" ] check run;
update the block that sets NCURSES_PATH from LDCONFIG_PATH accordingly.
Contributor
Author
|
/build |
Member
|
Hey @0chroma — good news, this is building now ✅ The Two small things to get it merge-ready once it's green:
Thanks for the GHC packaging cleanup! 🙌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR significantly simplifies the
ghcpackage by extracting its inline bootstrap compiler and Haskell-based build tools (alexandhappy) into their own dedicated, reusable packages.Changes
ghc-bootstrap(packages/ghc-bootstrap/build.ncl,packages/ghc-bootstrap/build.sh):x86_64andaarch64architectures.libtinfo.so.6, and linker configuration in an isolated way.alex(packages/alex/build.ncl,packages/alex/build.sh):alexlexical analyzer generator, built from source usingghc-bootstrap.happy(packages/happy/build.ncl,packages/happy/build.sh):happyparser generator, built from source usingghc-bootstrap.ghc(packages/ghc/build.ncl,packages/ghc/build.sh):alexandhappy.packages/ghc/build.shfrom 130 lines to 63 lines.ghcpackage now simply depends onghc-bootstrap,alex, andhappyas standard build dependencies.Verification
min check --packages ghc-bootstrap,alex,happy,ghcto verify Nickel schemas and formatting.min patched-build ghc-bootstrapto verify bootstrap compiler extraction and linking.min patched-build alexandmin patched-build happyto verify they compile successfully usingghc-bootstrap.Summary by CodeRabbit
New Features
Refactor