-
Notifications
You must be signed in to change notification settings - Fork 2.3k
chore: use default nsis template #6545
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
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.
Important
Looks good to me! 👍
Reviewed everything up to d55a0cd in 1 minute and 49 seconds. Click for details.
- Reviewed
1281lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/template-tauri-build-windows-x64-external.yml:49
- Draft comment:
NSIS template configuration via jq was removed. Ensure that removing the custom template update is deliberate and that the new installer hook covers all intended behaviors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment is asking the author to ensure that the removal of a custom template update is deliberate and that the new installer hook covers all intended behaviors. This is a request for confirmation, which violates the rules. However, it also points out a specific change (removal of NSIS template configuration) and suggests ensuring that the new installer hook covers all intended behaviors, which could be seen as a specific suggestion. The comment is borderline, but it leans towards being a request for confirmation.
2. .github/workflows/template-tauri-build-windows-x64.yml:95
- Draft comment:
Custom NSIS template modifications (including the generate_build_version function and sed replacements) have been removed and replaced with a signCommand update. Verify that the default NSIS template now supports all required customizations via hooks. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that the default NSIS template supports all required customizations via hooks. This falls under the category of asking the author to ensure behavior is intended or to double-check things, which is against the rules.
3. src-tauri/tauri.windows.conf.json:7
- Draft comment:
NSIS configuration updated to use installerHooks and installerIcon. Confirm that the new paths (‘./windows/hooks.nsh’ and ‘icons/icon.ico’) are correct and available in the build. - 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 the correctness and availability of paths in the build, which violates the rule against asking the author to confirm or ensure things. It doesn't provide a specific suggestion or point out a clear issue.
4. src-tauri/windows/hooks.nsh:1
- Draft comment:
New NSIS post-install hook added to check and install the VC++ Redistributable along with copying LICENSE and vulkan-1.dll. Consider verifying that the registry lookup (for key under VisualStudio 14.0) correctly targets the desired VC++ 2019 Redistributable. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment raises a valid technical concern about version mismatch. However, it's phrased as "Consider verifying..." which violates our rule about not asking authors to verify things. The issue should either be definitively stated as wrong (with evidence) or not mentioned at all. Additionally, the rest of the comment is just describing what the code does, which isn't useful. I might be wrong about the severity of the version mismatch - perhaps checking for 14.0 while downloading 17 is intentional or standard practice. Without deep knowledge of VC++ redistributable versioning practices, we can't be certain this is actually a problem. Following our principle of needing strong evidence, we should err on the side of removing the comment. The comment should be deleted as it's partly descriptive (not useful) and partly speculative (asking for verification without clear evidence of a problem).
Workflow ID: wflow_kfmfk9GxmCGt0NQX
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
d55a0cd to
2a905c6
Compare
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.
Caution
Changes requested ❌
Reviewed f1d97ac in 2 minutes and 14 seconds. Click for details.
- Reviewed
111lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/windows/hooks.nsh:3
- Draft comment:
The registry check uses the key for VisualStudio 14.0 runtimes. Verify that this key properly detects an existing installation of the newer VC++ redistributable (from VS 17) installed via vc_redist.x64.exe. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_jxuTiBV9yl46muKh
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 8ba0342 in 2 minutes and 2 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_bazgJYKB4GpDpKkO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM
Describe your changes
This pull request updates the Windows Tauri build process and installer configuration, focusing on improving post-installation setup, simplifying versioning and templating, and enhancing resource management. The changes introduce a custom NSIS installer hook for dependency installation and resource copying, streamline build scripts, and update configuration files to support these enhancements.
This would also allow to choose installation path on Windows
Chat UI
Model Providers
LLama.cpp imported and downloaded models
Hardware Settings
MCP Servers
Local API Server
HTTPS Proxy
App Updater
Fixes Issues
Important
Update Windows Tauri build process by removing custom NSIS template, adding installer hooks, and enhancing resource management.
src-tauri/tauri.bundle.windows.nsis.template.installerHooksintauri.windows.conf.jsonto usewindows/hooks.nshfor post-installation tasks.template-tauri-build-windows-x64-external.ymlandtemplate-tauri-build-windows-x64.ymlto remove NSIS template references and streamline versioning.hooks.nshinstalls VC++ Redistributable if not present and copiesLICENSEandvulkan-1.dllto install root.download-lib.mjsdownloads and copiesvc_redist.x64.exeandvulkan-1.dlltoresources/lib.This description was created by
for 8ba0342. You can customize this summary. It will automatically update as commits are pushed.