Fix executable discovery on Windows#12
Conversation
|
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 (2)
📝 WalkthroughWalkthroughRefactors binary detection into a staged pipeline: check Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Caller (initialize)"
participant Env as "Environment (FFMPEGCORE_<NAME>)"
participant System as "OS Lookup (which/where)"
participant FS as "Known Paths (filesystem)"
participant Config as "Configuration"
Caller->>Config: initialize -> detect_binary(name)
Config->>Env: check FFMPEGCORE_<NAME>
Env-->>Config: path or nil
alt env provided & executable
Config-->>Caller: return env path
else not provided or not executable
Config->>System: which/where lookup
System-->>Config: path or nil
alt system found & executable
Config-->>Caller: return system path
else not found/executable
Config->>FS: iterate known_paths (macOS/Linux/Windows)
FS-->>Config: path or nil
alt known path found & executable
Config-->>Caller: return known path
else none found
Config-->>Caller: raise BinaryNotFoundError (multi-OS install hints)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/ffmpeg_core/configuration.rb (2)
46-47: Logic is correct; consider multi-line for readability.The fix correctly appends
.exeon Windows whereFile.executable?requires the full filename. The inline conditional is functional but long. A multi-line format would improve readability.✨ Optional: multi-line format for clarity
- binary = if Gem.win_platform? then File.join(path, name + ".exe") else File.join(path, name) end + exe_name = Gem.win_platform? ? "#{name}.exe" : name + binary = File.join(path, exe_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ffmpeg_core/configuration.rb` around lines 46 - 47, The inline conditional that builds `binary` is correct but hard to read; refactor the expression "binary = if Gem.win_platform? then File.join(path, name + '.exe') else File.join(path, name) end" into a multi-line if/else block that assigns `binary` on separate lines (using `Gem.win_platform?`, `File.join(path, ...)`) and keep the existing `return binary if File.executable?(binary)` check unchanged to preserve behavior.
42-48: Windows-specific code path lacks test coverage.The
Gem.win_platform?branch is untested. The existing specs inspec/ffmpeg_core/configuration_spec.rbinitialize Configuration on the actual platform without stubbing, so CI (likely Linux/macOS) never exercises the.exelogic.Consider adding a unit test that stubs
Gem.win_platform?to returntrueand mocksFile.executable?to verify the correct path is checked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ffmpeg_core/configuration.rb` around lines 42 - 48, Add a unit test exercising the Windows branch of detect_binary: stub Gem.win_platform? to return true, stub ENV["PATH"] to a predictable value, and mock File.executable? to assert it is called with paths that append ".exe" (i.e., File.join(path, name + ".exe")); then initialize Configuration (or call detect_binary via the appropriate method) and assert the returned binary path is the expected .exe path. Target the spec in spec/ffmpeg_core/configuration_spec.rb and reference detect_binary, Gem.win_platform?, and File.executable? in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ffmpeg_core/configuration.rb`:
- Around line 46-47: The inline conditional that builds `binary` is correct but
hard to read; refactor the expression "binary = if Gem.win_platform? then
File.join(path, name + '.exe') else File.join(path, name) end" into a multi-line
if/else block that assigns `binary` on separate lines (using
`Gem.win_platform?`, `File.join(path, ...)`) and keep the existing `return
binary if File.executable?(binary)` check unchanged to preserve behavior.
- Around line 42-48: Add a unit test exercising the Windows branch of
detect_binary: stub Gem.win_platform? to return true, stub ENV["PATH"] to a
predictable value, and mock File.executable? to assert it is called with paths
that append ".exe" (i.e., File.join(path, name + ".exe")); then initialize
Configuration (or call detect_binary via the appropriate method) and assert the
returned binary path is the expected .exe path. Target the spec in
spec/ffmpeg_core/configuration_spec.rb and reference detect_binary,
Gem.win_platform?, and File.executable? in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6460f9b8-2d16-43db-ad99-f5e5bdf5783b
📒 Files selected for processing (1)
lib/ffmpeg_core/configuration.rb
|
@mapp113 I've improved your code a bit, check it please |
2 participants
On Windows,
File.executable?requires the full file name including the extension (e.g.,.exe). This PR updates the search logic to append the extension when running on Windows to ensure binaries are correctly located.Summary by CodeRabbit
New Features
Bug Fixes
Tests