-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
proto: Add version 0.54.1 #7527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new package manifest Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 |
|
Your changes did not pass all checks. Please address the issues in the manifest and comment starting with proto
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bucket/proto.json (1)
34-34: Consider adding a trailing newline.The file appears to be missing a trailing newline at the end. While not functionally impactful, adding a newline at EOF is a common convention that improves compatibility with various tools and diff outputs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bucket/proto.json
🔇 Additional comments (6)
bucket/proto.json (6)
1-5: LGTM!Version 0.54.1 is confirmed as the latest release of Proto. The metadata (description, homepage, license) is accurate and complete.
12-15: LGTM!The binary executables are correctly specified. Both
proto.exe(main CLI) andproto-shim.exe(tool shim) are essential for Proto's functionality.
16-19: LGTM!The environment configuration is well-designed:
PROTO_HOMEset to$persist_dirensures Proto's managed tools and configuration persist across Scoop updates.- Adding
$persist_dir\binto PATH makes proto-installed tools accessible system-wide.
20-22: LGTM!The
checkverconfiguration correctly uses the GitHub shorthand to detect new releases from the moonrepo/proto repository.
23-33: LGTM!The autoupdate configuration is correct:
- URL template properly uses
$versionplaceholder- Hash extraction from
$url.sha256with regex for 64-character hex digest is appropriate- Note: If ARM64 support is added to the
architecturesection, it should also be added here for consistency.
6-11: No changes needed. The manifest correctly includes only the available Windows build for proto v0.54.1. The GitHub release does not provide an ARM64 Windows build (aarch64-pc-windows-msvc); ARM64 builds are limited to Linux and macOS. The SHA256 hash is verified as correct against the official release checksum.Likely an incorrect or invalid review comment.
|
/verify |
|
All changes look good. Wait for review from human collaborators. proto
|
The current Lines 12 to 14 in 5e985e0
|
It doesn't seems a problem, where |
|
I am against calling proto setup directly via CLI, since it will not only modify the PATH, but also the terminal profile. And I did not see a command to reverse the effect by the command, which will left unmanaged env var and scripts in PROFILE, in this case. |
From my memory, full file support in persist is still pending review, so this might not work reliably yet. If you have tested this, and it behaves correctly in your environment, that would be good to confirm. |
It's the official installation procedure of install script, which is the workflow of proto itself canonically. Quote from moonrepo
|
No, that uninstall note applies to the generic install flow, not to proto setup specifically, and setup is effectively invoking proto activate behind the scenes.
which introduces additional, persistent side effects that are not reverted by the documented uninstall steps. |
4db780a to
63db674
Compare
|
Here the manifest updated, sadly, due to its special property, the script is necessary in following reasons. Reasons:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bucket/proto.json`:
- Around line 23-35: The uninstaller's Remove-Item call is too aggressive—don't
delete the entire $protoHome; instead modify the uninstaller script (in the
"uninstaller" block) to only remove the installed binary and any shim created by
the installer: target the proto executable under $binDir and any shim files
under $shimsDir (use $binDir and $shimsDir variables), remove those specific
files, and only remove $protoHome if it was created by this installer and is
empty; replace the current Remove-Item -Path $protoHome -Recurse -Force with
targeted removals of the binary/shims and a safe empty-directory removal check.
♻️ Duplicate comments (1)
bucket/proto.json (1)
12-21:protomay be unreachable after install (no shim/PATH addition).
proto.exeis moved out of$dir, and the manifest doesn’t definebinorenv_add_path. Withproto setup --no-modify-profile, PATH may remain unchanged, leaving no executable on PATH after install. Please verify thatprotoends up discoverable on Windows; if it doesn’t, add explicit PATH/shim handling or adjust the install flow.
Closes: #7526
<manifest-name[@version]|chore>: <general summary of the pull request>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.