lazarus: Refactor installation logic, add file associations#17765
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR updates the lazarus bucket manifest and adds two registry scripts. The manifest switches license/checksum URLs to HTTPS, replaces inline PowerShell FPC config generation with an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
bucket/lazarus.json (1)
69-85: Please run the standard Scoop checks before merge.For this manifest, I’d verify with:
scoop config debug true .\bin\checkver.ps1 -App lazarus -f .\bin\formatjson.ps1 -App lazarus scoop install .\bucket\lazarus.json -a 64bit scoop install .\bucket\lazarus.json -a 32bitThe contribution guide and manifest wiki are also worth a quick pass if you have not already. As per coding guidelines
Provide clear instructions for testing the manifest locally before submission: ... .\bin\checkver.ps1 -App <manifest-name> -f ... .\bin\formatjson.ps1 -App <manifest-name> ... scoop install <manifest-path> -a <architecture>.🤖 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 `@bucket/lazarus.json` around lines 69 - 85, Run the standard Scoop validation steps for the lazarus.json manifest: execute checkver against the "checkver" block (use .\bin\checkver.ps1 -App lazarus -f with debug enabled), run .\bin\formatjson.ps1 -App lazarus to ensure the autoupdate/architecture and hash sections are well-formed, and verify installation for both architectures by installing the manifest for 64bit and 32bit to confirm the "autoupdate.architecture.64bit.url" and "autoupdate.architecture.32bit.url" templates and the "hash.regex" work as expected; fix any formatting, regex or URL template issues flagged by these tools before merging.
🤖 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 `@bucket/lazarus.json`:
- Around line 9-12: Update the notes entry so users know they must run the reg
import command from an elevated/admin shell when the package was installed with
-g (global), because post_install rewrites the association file to
HKEY_LOCAL_MACHINE when $global is true and a non-elevated reg import will fail;
locate the notes array in bucket/lazarus.json and modify the string "reg import
\"$dir\\install-associations.reg\"" to explicitly mention running as
administrator (or using an elevated shell) when installing with -g.
- Around line 24-30: The installer script must abort when fpcmkcfg.exe is
missing or when it returns non‑zero: after the Get-ChildItem ... | Select-Object
-First 1 pipeline, check that $executable_dir is not null/empty and throw a
clear error (or call exit) if it is; when invoking Start-Process for
"$executable_dir\\fpcmkcfg.exe" use -PassThru to capture the process object and
after Wait verify the process.ExitCode and throw/exit with a descriptive message
if ExitCode is non‑zero so the install fails properly; update references to
$executable_dir, $fpc_dir and $process_args accordingly so downstream code does
not run on a missing executable or failed command.
In `@scripts/lazarus/uninstall-associations.reg`:
- Around line 5-11: The uninstall script currently deletes registry keys it
never created (specifically -HKEY_CURRENT_USER\Software\Lazarus and removing
"Lazarus" under [HKEY_CURRENT_USER\Software\RegisteredApplications]); change it
to only remove keys that this package created during install (the
Software\Classes\... entries or any capability keys you explicitly add). Update
the install step to create explicit capability keys if you need Default Programs
cleanup, and modify uninstall to remove exactly those capability keys (and the
specific Software\Classes subkeys) rather than wiping HKCU\Software\Lazarus or
the generic RegisteredApplications "Lazarus" value.
---
Nitpick comments:
In `@bucket/lazarus.json`:
- Around line 69-85: Run the standard Scoop validation steps for the
lazarus.json manifest: execute checkver against the "checkver" block (use
.\bin\checkver.ps1 -App lazarus -f with debug enabled), run .\bin\formatjson.ps1
-App lazarus to ensure the autoupdate/architecture and hash sections are
well-formed, and verify installation for both architectures by installing the
manifest for 64bit and 32bit to confirm the "autoupdate.architecture.64bit.url"
and "autoupdate.architecture.32bit.url" templates and the "hash.regex" work as
expected; fix any formatting, regex or URL template issues flagged by these
tools before merging.
🪄 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: 4b66d8f0-4605-49fb-ab0e-52e507e72bed
📒 Files selected for processing (3)
bucket/lazarus.jsonscripts/lazarus/install-associations.regscripts/lazarus/uninstall-associations.reg
Contributor
Author
|
/verify |
Contributor
|
All changes look good. Wait for review from human collaborators. lazarus
|
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
Refactors the
lazarusmanifest to improve FPC configuration reliability and introduces automatic registry generation for file associations.Related issues or pull requests
Changes
post_installwith a dynamic search forfpcmkcfg.exe. This ensures the compiler configuration (fpc.cfg) is generated correctly regardless of version-specific subfolder naming..pas,.inc,.lpr,.lpi, etc.).lazarus.exein thebinarray.Testing
The test results are as follows:
<manifest-name[@version]|chore>: <general summary of the pull request>