grepwin: Fix portability#17697
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
📝 WalkthroughWalkthroughThe grepwin manifest renames the portable executable to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bucket/grepwin.json (1)
1-63: Please run the standard Scoop manifest verification flow before merge.Recommended local checks:
scoop config debug true scoop config gh_token <your-github-token> # optional, read-only .\bin\checkver.ps1 -App grepwin -f .\bin\formatjson.ps1 -App grepwin scoop install .\bucket\grepwin.json -a 64bit scoop uninstall grepwin scoop install .\bucket\grepwin.json -a 32bitAnd for this PR specifically, verify:
grepWinshim launchesgrepWin_portable.exe- direct EXE launch uses INI persistence
reg import "$dir\install-context.reg"context-menu launch also uses INI persistenceAs per coding guidelines: "Provide clear instructions for testing the manifest locally before submission."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/grepwin.json` around lines 1 - 63, Run the standard local verification and functional tests for this manifest: enable debug and (optionally) gh_token, then execute the provided checkver and formatjson scripts (referencing checkver and autoupdate entries) for grepwin.json, install the manifest in both architectures via the scoop install command to validate pre_install and post_install behavior (verify pre_install creates grepwin.ini and renames exe to grepWin_portable.exe, post_install correctly substitutes $app_path into install-context.reg/uninstall-context.reg and writes them to $dir), confirm the shim "grepWin" launches grepWin_portable.exe, launching the EXE directly persists settings to grepwin.ini, and importing "$dir\\install-context.reg" adds the context menu that also uses the same INI persistence; fix any issues discovered in pre_install, post_install, bin/shortcuts, persist, or the reg files before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bucket/grepwin.json`:
- Around line 1-63: Run the standard local verification and functional tests for
this manifest: enable debug and (optionally) gh_token, then execute the provided
checkver and formatjson scripts (referencing checkver and autoupdate entries)
for grepwin.json, install the manifest in both architectures via the scoop
install command to validate pre_install and post_install behavior (verify
pre_install creates grepwin.ini and renames exe to grepWin_portable.exe,
post_install correctly substitutes $app_path into
install-context.reg/uninstall-context.reg and writes them to $dir), confirm the
shim "grepWin" launches grepWin_portable.exe, launching the EXE directly
persists settings to grepwin.ini, and importing "$dir\\install-context.reg" adds
the context menu that also uses the same INI persistence; fix any issues
discovered in pre_install, post_install, bin/shortcuts, persist, or the reg
files before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8f8ac72-5506-4c7f-b981-2674a6c38d82
📒 Files selected for processing (1)
bucket/grepwin.json
|
/verify |
|
All changes look good. Wait for review from human collaborators. grepwin
|
z-Fng
left a comment
There was a problem hiding this comment.
A workaround is needed to replace the executable file name in the registry for existing users.
|
/verify |
|
All changes look good. Wait for review from human collaborators. grepwin
|
|
Oh, right—when I reverted the changes I had initially made to the registry, I forgot to fix the typo in the context menu; instead of |
Feel free to submit another PR for this. |
To properly handle portability, by renaming the executable to
grepWin_portable.exe, the alternative solution would have been to add the argument to the “script” registry key, but that would not have covered the case where the executable is launched directlyCloses #17696
<manifest-name[@version]|chore>: <general summary of the pull request>