Fix dep resolution and --HEAD flag leak in jdx-package#21
Conversation
Two bugs in jdx-package.rb: 1. Dependency.expand includes glibc@2.17 and linux-headers@4.4 even when YJIT is enabled (the default). The formula conditionally declares these deps with `if build.without? "yjit"`, but the dep resolver doesn't honor build options. This causes the build to fail when glibc@2.17's post-install localedef step errors out. Fix by pruning these deps when --without-yjit is not passed. 2. The `flags` array is defined before the `args.named.each` loop and `--HEAD` is appended inside the loop without being removed. When processing multiple formulae, `--HEAD` accumulates across iterations. Fix by using `flags.dup` per iteration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two critical bugs within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 refactors the handling of installation flags in cmd/jdx-package.rb by making the --HEAD flag conditional per named argument. It also introduces a new dependency pruning rule to exclude glibc@ and linux-headers@4.4 when YJIT is enabled. A suggestion was made to improve readability and performance by hoisting the !args.without_yjit? check out of the dependency expansion loop.
| deps = Dependency.expand(Formula[name], cache_key: "jdx-package-#{name}") do |_dependent, dep| | ||
| Dependency.prune if dep.test? || dep.optional? | ||
| Dependency.prune if dep.name == "rustup" && args.without_yjit? | ||
| Dependency.prune if !args.without_yjit? && (dep.name.start_with?("glibc@") || dep.name == "linux-headers@4.4") |
There was a problem hiding this comment.
The condition !args.without_yjit? is re-evaluated for every dependency within this loop. For better readability and a minor performance improvement, consider hoisting this check out of the Dependency.expand block.
You could define a variable like yjit_enabled = !args.without_yjit? before the args.named.each loop and use it here:
Dependency.prune if yjit_enabled && (dep.name.start_with?("glibc@") || dep.name == "linux-headers@4.4")
1 participant
Summary
Two bugs in
cmd/jdx-package.rb:glibc@2.17 included when YJIT is enabled:
Dependency.expandincludesglibc@2.17andlinux-headers@4.4even when YJIT is enabled (the default). The formula conditionally declares these deps withif build.without? "yjit", but the dep resolver doesn't honor build options. This causes the build to fail whenglibc@2.17's post-installlocaledefstep errors out. Fix by pruning these deps when--without-yjitis not passed.--HEADflag leaks across loop iterations: Theflagsarray is defined before theargs.named.eachloop and--HEADis appended inside the loop without being removed. When processing multiple formulae,--HEADaccumulates across iterations. Fix by usingflags.dupper iteration.Test plan
glibc@2.17is conditional onbuild.without? "yjit")jdx-ruby@3.4.7with YJIT on Linux — should no longer pull inglibc@2.17--HEADshould not leak🤖 Generated with Claude Code
Note
Medium Risk
Moderate risk because it changes dependency pruning and build flags for bottle builds, which can affect build outputs and CI behavior, but it’s localized to
cmd/jdx-package.rb.Overview
Fixes
jdx-packagepackaging to avoid state leaking across formula builds by duplicating per-formula flags so--HEADis applied only for the currentname.Adjusts dependency expansion to explicitly prune
glibc@*andlinux-headers@4.4when YJIT is enabled (i.e.,--without-yjitis not set), preventing unintended deps from being built/installed during bottle creation.Written by Cursor Bugbot for commit 428fead. This will update automatically on new commits. Configure here.