librecad: Update to version 2.2.1.4, fix checkver & autoupdate#17533
Conversation
WalkthroughUpdates LibreCAD Scoop manifest from version 2.2.1.2 to 2.2.1.4, changes license format, updates GitHub release-based checkver/autoupdate configuration, and adds file association registration for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bucket/librecad.json (1)
33-37: Prefer explicit.regallowlist over wildcard discovery/import.Using
Get-ChildItem ... '*.reg'(Line 33) andun*.regimports (Line 49) is broad. An explicit file list is safer and prevents accidental processing if extra registry files are added later.Recommended refactor
- "Get-ChildItem -Path \"$bucketsdir\\$bucket\\scripts\\$app\" -Filter '*.reg' -File | ForEach-Object {", + "'install-associations', 'uninstall-associations' | ForEach-Object {", + " $regPath = \"$bucketsdir\\$bucket\\scripts\\$app\\$_.reg\"", + " if (!(Test-Path $regPath)) { return }", - " $content = Get-Content -Path $_.FullName -Encoding utf8", + " $content = Get-Content -Path $regPath -Encoding utf8", " if ($global) { $content = $content -replace 'HKEY_CURRENT_USER', 'HKEY_LOCAL_MACHINE' }", - " $content -replace '{{librecad_dir}}', $librecad_dir | Set-Content -Path \"$dir\\$($_.Name)\" -Encoding unicode", + " $content -replace '{{librecad_dir}}', $librecad_dir | Set-Content -Path \"$dir\\$_.reg\" -Encoding unicode", "}"Also applies to: 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/librecad.json` around lines 33 - 37, The script currently uses wildcard discovery (Get-ChildItem -Filter '*.reg') and pattern-based imports (un*.reg), which is unsafe; replace these with an explicit allowlist of registry filenames: define an array (e.g., $regAllowlist) containing the exact .reg filenames to process for the given $bucket/$app, then iterate that array instead of using Get-ChildItem and use the same allowlist when importing/unregistering (instead of 'un*.reg'); update places referencing $bucketsdir, $bucket, $app, $librecad_dir, $dir and the import/unregister logic to read from the explicit list and handle missing files gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/librecad.json`:
- Around line 32-36: The $librecad_dir assignment currently uses -replace
'\\\\','\\\\' which is a no-op; update the code that prepares the registry path
variable ($librecad_dir) to properly escape backslashes by using the
.Replace('\\','\\\\') method (mirroring vscode.json/notepadplusplus.json) so
that when '{{librecad_dir}}' is substituted in the Get-ChildItem/ForEach-Object
block and written via Set-Content the registry string contains doubled
backslashes and is valid.
---
Nitpick comments:
In `@bucket/librecad.json`:
- Around line 33-37: The script currently uses wildcard discovery (Get-ChildItem
-Filter '*.reg') and pattern-based imports (un*.reg), which is unsafe; replace
these with an explicit allowlist of registry filenames: define an array (e.g.,
$regAllowlist) containing the exact .reg filenames to process for the given
$bucket/$app, then iterate that array instead of using Get-ChildItem and use the
same allowlist when importing/unregistering (instead of 'un*.reg'); update
places referencing $bucketsdir, $bucket, $app, $librecad_dir, $dir and the
import/unregister logic to read from the explicit list and handle missing files
gracefully.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4562d875-b146-4140-865c-892134cfee84
📒 Files selected for processing (3)
bucket/librecad.jsonscripts/librecad/install-associations.regscripts/librecad/uninstall-associations.reg
|
/verify |
|
All changes look good. Wait for review from human collaborators. librecad
|
z-Fng
left a comment
z-Fng
left a comment
There was a problem hiding this comment.
The changes in this PR look good and work as expected. However, I have a few questions:
- Phrasing: I believe "
For file associations, run:" is more concise and idiomatic. - Wildcards: Specifying the .reg scripts explicitly was clearer. What's the rationale for switching to wildcards?
- Process Execution: Why use Start-Process to run the registry scripts? Does it offer any specific advantages over the previous method?
Why not keep these consistent with other manifests? Inconsistency across manifests can be quite confusing for new contributors. Also, I don't see the necessity of refactoring scripts that are already straightforward and easy to verify. Changing them to a more complex structure doesn't seem to offer much benefit, but it does make the code harder to audit at a glance.
I think it is not sufficiently formal.
If there are many
It does not provide any real advantage. Scoop is implemented in PowerShell, and although
I personally place greater emphasis on general applicability. This allows contributors to reuse these scripts without needing to consider various specific scenarios, such as modifying parts of the script due to different file names or updating the script whenever a |
These scripts aren't generalized enough for universal use yet. Besides, using wildcards in the installation directory could introduce many unforeseen issues. IMO improving general applicability isn't as important as maintaining clarity and intuitiveness. I believe making it easy to understand at a glance benefits both contributors and reviewers. But since it works, let's merge it for now... We still have a bunch of issues to discuss anyway. ScoopInstaller/Main#7061 |
Sorry, I do not fully understand why you consider In my design, when the application contents are located in When the application contents are located in
Admittedly, such a situation could occur, but I believe this approach still covers the vast majority of cases. Generality does not mean covering every possible scenario; being applicable to most cases is sufficient. The current script only retrieves files under
I think that clarity and intuitiveness are closely related to personal habits. While using wildcards may be less explicit than specifying file names in certain situations, this does not necessarily mean that wildcards are inherently unclear or unintuitive. For contributors, many may only contribute occasionally and may not be familiar with PowerShell syntax. Using more general approaches allows them to apply these scripts in most cases without needing to adjust them for specific circumstances. In my view, this lowers the barrier to contribution to some extent, as contributors do not need to fully understand the script or modify it based on file names; they can simply copy and use it. For reviewers, such a general approach effectively provides a standardized template. It may cause some confusion during the initial review, but once the pattern is recognized, manifests that follow the same template no longer require extensive verification. In addition, when this approach is used, adding or removing multiple |
2 participants
Summary
Updates
librecadto version 2.2.1.4 and introduces automated registry-based file association for.dxffiles.Changes
licensefield with structured SPDX identifier and URLnotesto guide users on how to manually register file associationssuggestforvcredist2022as a recommended dependencypre_installscript to clean up residual installer files ($*,Uninst*,vc_redist*)post_installanduninstallerscripts:.regfiles by replacing{{librecad_dir}}placeholders with the actual installation path.HKCUtoHKLM.checkverto use GitHub API with complex regex to handle MSVC-based asset naming conventions and version tags.autoupdateto align with the new asset URL patterns using$matchTagand$matchString.Testing
The test results are as follows:
<manifest-name[@version]|chore>: <general summary of the pull request>Summary by CodeRabbit
Release Notes
New Features
Chores