Normalize ragel config handling in extconf #2
Conversation
WalkthroughAdds Ragel dependency notes to README for git-based installs and documents regeneration of the HTTP parser. Updates ext/unicorn_http/extconf.rb to auto-generate unicorn_http.c via Ragel when missing, aborting if Ragel is unavailable. The new helper runs before makefile generation without altering existing feature checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Ruby as ruby ext/unicorn_http/extconf.rb
participant Ext as extconf.rb
participant FS as Filesystem
participant Ragel as ragel
Dev->>Ruby: Run extconf.rb
Ruby->>Ext: Execute
Ext->>FS: Check unicorn_http.c exists?
alt C exists
Ext-->>Ruby: Skip generation
else C missing
Ext->>Ext: Resolve ragel path (config or PATH)
alt Ragel found
Ext->>Ragel: Generate unicorn_http.c (-G2 -T0 ...)
Ragel-->>Ext: Exit status
alt Success
Ext->>FS: Write unicorn_http.c
else Failure
Ext-->>Ruby: abort("ragel failed")
end
else Ragel missing
Ext-->>Ruby: abort("ragel not found")
end
end
Ruby-->>Dev: Continue makefile generation or abort
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ext/unicorn_http/extconf.rb (3)
9-16: Handle dir/relative values and validate executability for --with-ragelIf users pass a directory (e.g., --with-ragel=/usr/local/bin) or a non-absolute/relative path, we should resolve it and ensure it’s an executable. This also tightens validation before falling back to PATH.
Apply this diff:
ragel_cfg = with_config('ragel') { find_executable('ragel') } ragel = if ragel_cfg.respond_to?(:strip) ragel_cfg = ragel_cfg.strip ragel_cfg unless ragel_cfg.empty? || (ragel_cfg.is_a?(String) && ragel_cfg !~ %r{\A/}) end - ragel ||= find_executable('ragel') + # If a directory is provided, look for 'ragel' inside it + if ragel && File.directory?(ragel) + ragel = File.join(ragel, 'ragel') + end + # Only accept a real executable; otherwise fall back to PATH lookup + ragel = nil unless ragel && File.file?(ragel) && File.executable?(ragel) + ragel ||= find_executable('ragel')
5-8: Regenerate only when unicorn_http.c is missing or staleCurrently we skip regeneration if unicorn_http.c exists, even if unicorn_http.rl is newer. Consider basic mtime check to avoid stale parser sources.
def generate_ragel_source src = File.expand_path('unicorn_http.c', __dir__) - return if File.exist?(src) + rl = File.expand_path('unicorn_http.rl', __dir__) + return if File.exist?(src) && (!File.exist?(rl) || File.mtime(src) >= File.mtime(rl))
17-23: Augment error with a direct --with-ragel usage hintSmall UX improvement: tell users how to pass an absolute path/dir to ragel, in addition to gmake/package manager guidance.
unless ragel abort <<~MSG ragel(1) is required to generate ext/unicorn_http/unicorn_http.c. Install ragel (e.g. via your package manager) or run `gmake ragel` from the repository root before installing unicorn from git. + Alternatively, point extconf at your ragel via: + ruby ext/unicorn_http/extconf.rb --with-ragel=/path/to/ragel + ruby ext/unicorn_http/extconf.rb --with-ragel=/path/to/bin MSG endREADME (1)
94-100: Clarify regeneration behavior to avoid confusionextconf regenerates only if the C source is missing (or stale, if we add the mtime check). Wording “will regenerate” may imply it always does. Suggest softening to “(re)generate if needed”.
-`ruby ext/unicorn_http/extconf.rb`, which will regenerate -`ext/unicorn_http/unicorn_http.c` and report an error if Ragel is -missing. +`ruby ext/unicorn_http/extconf.rb`, which will (re)generate +`ext/unicorn_http/unicorn_http.c` if needed and report an error if Ragel is +missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
README(1 hunks)ext/unicorn_http/extconf.rb(1 hunks)
🧰 Additional context used
🪛 RuboCop (1.80.2)
ext/unicorn_http/extconf.rb
[convention] 5-30: Cyclomatic complexity for generate_ragel_source is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 5-30: Perceived complexity for generate_ragel_source is too high. [9/8]
(Metrics/PerceivedComplexity)
🔇 Additional comments (1)
ext/unicorn_http/extconf.rb (1)
5-30: LGTM: PATH fallback when passing bare --with-ragelThe normalization works: boolean/empty --with-ragel falls back to PATH; absolute paths are honored; shell injection is avoided via system(*cmd).
Please verify the following manual scenarios:
- No args (ragel in PATH): extconf generates when .c is missing.
- --with-ragel (boolean): same as above (PATH lookup).
- --with-ragel=/absolute/path/to/ragel: uses that binary.
- --with-ragel=/some/bin (dir): after applying the suggested refactor, uses /some/bin/ragel.
1 participant
Summary
--with-ragelwithout a path falls back to PATH lookupTesting
https://chatgpt.com/codex/tasks/task_b_68d6ee1a3904832aaef4059154f6bc2b
Summary by CodeRabbit