-
Notifications
You must be signed in to change notification settings - Fork 182
windows packaging 2 #260
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: main
Are you sure you want to change the base?
windows packaging 2 #260
Conversation
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.
❌ Changes requested. Reviewed everything up to 49b4895 in 2 minutes and 38 seconds
More details
- Looked at
512lines of code in1files - Skipped
3files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. scripts/pearai/build-package-pearai-win-x64.ps1:54
-
Draft comment:
Consider extending the existingInvoke-CMDfunction to add the success message functionality instead of creating a duplicate. -
function
Invoke-CMD(setup-environment.ps1) -
Reason this comment was not posted:
Comment looked like it was already resolved.
2. scripts/pearai/build-package-pearai-win-x64.ps1:91
- Draft comment:
Avoid using a leading slash in the ChildPath for Join-Path. It may cause path issues on Windows (e.g. '/extensions/pearai-ref'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Join-Path is designed to handle path separators automatically. While it's generally cleaner style to avoid leading slashes with Join-Path, both forms will work correctly on Windows. The leading slash doesn't cause actual issues. This seems more like a style preference than a real problem.
The comment could be correct that it's better style to avoid leading slashes, even if it works fine either way. Maybe there are edge cases where the leading slash matters that I'm not aware of.
While cleaner style is good, the current code works perfectly fine and the change would be purely cosmetic. Join-Path is specifically designed to handle path separators correctly regardless of leading slashes.
This comment suggests a style improvement but doesn't identify a real functional issue. The code works correctly either way.
3. scripts/pearai/build-package-pearai-win-x64.ps1:149
- Draft comment:
Comparing the full trimmed output of 'git status --porcelain' to a specific hard-coded string is brittle. Consider parsing status per file instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment has a valid point - comparing full git status output to a hardcoded string is fragile since the order of files could change, or there could be whitespace differences. A more robust approach would be to parse the status output and check for specific files. However, this is a build script that's working as intended, and the risk is fairly low since it's just checking for specific known files to ignore.
The comment identifies a real potential issue, but may be overstating its importance given this is just a build script checking for specific files to ignore. The current approach, while not ideal, is functional and clear.
While the suggested improvement would be more robust, the current code is simple and working. The risk of the git status output changing format is low, and the impact would just be failing to ignore those specific files.
Given this is a working build script and the risk/impact is low, this comment, while technically correct, is not important enough to warrant a code change.
4. scripts/pearai/build-package-pearai-win-x64.ps1:365
- Draft comment:
Consider verifying that the file 'windows-python-2023-extensions.zip' exists before calling Expand-Archive to prevent unexpected failures. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. scripts/pearai/build-package-pearai-win-x64.ps1:7
- Draft comment:
Typo: On line 7, use "don't" instead of "dont" for proper grammar. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
While technically correct, this is an extremely minor typo in a comment. Comments are documentation, not code. The meaning is perfectly clear either way. This kind of nitpick about apostrophes in comments does not improve code quality or functionality in any meaningful way.
The typo does make the documentation slightly less professional looking. Some teams may have strict documentation standards.
Even with strict documentation standards, an apostrophe in a comment is too minor to warrant a PR comment. This creates noise and distracts from more important issues.
Delete this comment. It's an extremely minor documentation nitpick that doesn't meaningfully improve the code.
Workflow ID: wflow_161iss0K9NqdYHXK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
|
|
||
|
|
||
| $pearaiRefDir = Join-Path -Path $pearaiDir -ChildPath "/extensions/pearai-ref" |
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.
Avoid using a leading '/' in ChildPath with Join-Path. This creates an absolute path instead of appending to $pearaiDir. For example, use 'extensions/pearai-ref' instead of '/extensions/pearai-ref'.
| $pearaiRefDir = Join-Path -Path $pearaiDir -ChildPath "/extensions/pearai-ref" | |
| $pearaiRefDir = Join-Path -Path $pearaiDir -ChildPath "extensions/pearai-ref" |
| } | ||
| } | ||
| cd $pearaiSubmoduleDir | ||
| git checkout main |
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.
Unconditionally checking out 'main' in the submodule overrides any custom commit hash provided via Input_SubmoduleCommitHash. Consider conditionally executing git checkout main only if no custom commit hash was specified.
| git checkout main | |
| if (-not $Input_SubmoduleCommitHash) { git checkout main } |
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.
❌ Changes requested. Incremental review on d6aa3d2 in 1 minute and 9 seconds
More details
- Looked at
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/build-package-win-x64.yml:13
- Draft comment:
The new 'roo-code-hash' input is declared but not used later in the workflow. If this input is needed for building, please assign and pass it to the appropriate script. - Reason this comment was not posted:
Marked as duplicate.
2. .github/workflows/build-package-win-x64.yml:106
- Draft comment:
Removal of customPearappVersion and forceBuild variables from the testenv.ps1 call: confirm that testenv.ps1 has been updated accordingly and no dependencies on these parameters remain. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that a script has been updated and that there are no dependencies on removed parameters. This falls under asking the author to double-check things, which is not allowed.
Workflow ID: wflow_QmdnTDU8e1Y1cX5B
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can teach Ellipsis what to look for with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| submodule-commit-hash: | ||
| description: 'Submodule commit hash to build (optional)' | ||
| required: false | ||
| roo-code-hash: |
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.
The new input roo-code-hash is defined but not used in the subsequent steps. Consider passing it to the script or removing it if not needed.
Important
Add GitHub Actions workflow and PowerShell script for building Windows x64 PearAI packages with manual triggers and environment setup.
build-package-win-x64.ymlfor building Windows x64 packages.pearapp,submodule, androo-code.build-package-pearai-win-x64.ps1for building PearAI app and submodules.rcedit.exe.This description was created by
for d6aa3d2. You can customize this summary. It will automatically update as commits are pushed.