Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10b82aa3c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| set "SIGNING_ENABLED=0" | ||
| if defined THEMETOGGLE_SIGN_PFX_PATH set "SIGNING_ENABLED=1" | ||
| if defined THEMETOGGLE_SIGN_CERT_THUMBPRINT set "SIGNING_ENABLED=1" | ||
| if defined PFX_PATH set "SIGNING_ENABLED=1" |
There was a problem hiding this comment.
This marks signing as enabled when only PFX_PATH is set, but tools/signing/sign-release.ps1 does not read PFX_PATH (it only accepts THEMETOGGLE_SIGN_PFX_PATH or THEMETOGGLE_SIGN_CERT_THUMBPRINT). In that situation the build-release flow will try to sign and the script will abort with “Set THEMETOGGLE_SIGN_PFX_PATH…”, so users following the newly documented PFX_PATH env var will see a failing release build. Either remove this check or wire PFX_PATH through to the signing script so the flag reflects actual support.
Useful? React with 👍 / 👎.
| REM Optional: Sign | ||
| if %DO_SIGN%==1 ( | ||
| echo Signing ThemeToggle.exe... | ||
| powershell -NoProfile -ExecutionPolicy Bypass -File "tools\signing\sign-release.ps1" -Target exe |
There was a problem hiding this comment.
When /sign is used, the signing step runs but its exit code is ignored. If the signing script fails (e.g., missing cert, bad password, missing signtool), build.bat still exits with code 0, so downstream release steps can publish an unsigned binary while reporting success. Capturing %ERRORLEVEL% after the PowerShell call and exiting non‑zero would prevent silent unsigned builds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Release prep for v1.5.4 focused on documentation/tooling improvements and packaging metadata updates.
Changes:
- Bumped version references to 1.5.4 across installer/resources/VERSION and updated WinGet manifests.
- Added repo inventory tooling (
tools/inventory.ps1) and linked/generated documentation (docs/INVENTORY.md, docs index update). - Reworked Windows batch release/build scripts and added a VS toolchain detection helper (
tools/test_vcvars.bat).
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| winget/SevIQ.ThemeToggle.yaml | Bumps WinGet manifest version to 1.5.4. |
| winget/SevIQ.ThemeToggle.locale.en-US.yaml | Updates WinGet locale version + ReleaseNotesUrl for 1.5.4. |
| winget/SevIQ.ThemeToggle.installer.yaml | Updates WinGet installer version/date/URL/SHA for 1.5.4. |
| tools/test_vcvars.bat | Adds helper script to probe common vcvarsall.bat locations. |
| tools/signing/README.md | Updates signing docs for 1.5.4 and adds env-var notes. |
| tools/inventory.ps1 | Adds script to generate a markdown inventory of tracked files and folder stats. |
| setup.nsi | Bumps NSIS product version to 1.5.4. |
| docs/RELEASE_NOTES.md | Adds v1.5.4 release notes entry. |
| docs/README.md | Adds link to generated INVENTORY.md. |
| docs/INVENTORY.md | Adds generated repository inventory report. |
| dist/build-release.bat | Simplifies release build pipeline steps and signing/packaging flow. |
| build.bat | Adds /sign option and updates build output messaging/VS detection paths. |
| VERSION | Bumps repo version file to 1.5.4. |
| ThemeToggle.rc | Updates VERSIONINFO fields to 1.5.4.0. |
| Resources/ThemeToggle.manifest | Updates assembly identity version to 1.5.4.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| REM Optional: Sign | ||
| if %DO_SIGN%==1 ( | ||
| echo Signing ThemeToggle.exe... | ||
| powershell -NoProfile -ExecutionPolicy Bypass -File "tools\signing\sign-release.ps1" -Target exe | ||
| ) | ||
|
|
||
| REM Restore original directory | ||
| echo Build completed successfully! | ||
| popd | ||
| exit /b 0 |
There was a problem hiding this comment.
When /sign is used, the signing step does not check the exit code from sign-release.ps1, and the script still prints "Build completed successfully!" and exits 0 even if signing fails. Check %ERRORLEVEL% after the PowerShell call and return non-zero on failure so callers (like dist\build-release.bat) can fail the build appropriately.
| The signing script also accepts common env var names used by some local helpers: | ||
| ``` | ||
| setx PFX_PATH "C:\path\to\cert.pfx" | ||
| setx PFX_PASS "your-password" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This README claims sign-release.ps1 accepts PFX_PATH / PFX_PASS, but the script currently only reads THEMETOGGLE_SIGN_PFX_PATH and THEMETOGGLE_SIGN_PFX_PASSWORD (or THEMETOGGLE_SIGN_CERT_THUMBPRINT). Either update sign-release.ps1 to support these aliases or remove this documentation to avoid misleading users.
| The signing script also accepts common env var names used by some local helpers: | |
| ``` | |
| setx PFX_PATH "C:\path\to\cert.pfx" | |
| setx PFX_PASS "your-password" | |
| ``` |
| | |-- build-release.bat | ||
| | \-- update-winget.ps1 | ||
| |-- docs/ | ||
| | |-- DEVELOPMENT.md | ||
| | |-- README.md | ||
| | |-- RELEASE.md | ||
| | |-- RELEASE_NOTES.md | ||
| | \-- TECHNICAL.md | ||
| |-- include/ | ||
| | |-- BroadcastManager.h | ||
| | |-- RegistryManager.h | ||
| | |-- StringUtils.h | ||
| | |-- Types.h | ||
| | \-- UxThemeHelper.h | ||
| |-- Resources/ | ||
| | |-- ThemeToggle.ico | ||
| | \-- ThemeToggle.manifest | ||
| |-- src/ | ||
| | |-- BroadcastManager.cpp | ||
| | |-- main.cpp | ||
| | |-- RegistryManager.cpp | ||
| | \-- UxThemeHelper.cpp | ||
| |-- tools/ | ||
| | |-- signing/ | ||
| | | |-- README.md | ||
| | | \-- sign-release.ps1 | ||
| | |-- bench.ps1 | ||
| | |-- bump-version.ps1 | ||
| | |-- cleanup.bat | ||
| | |-- export-bench.ps1 | ||
| | |-- export-source.ps1 | ||
| | |-- release.ps1 | ||
| | |-- test_vcvars.bat | ||
| | \-- validate.bat |
There was a problem hiding this comment.
docs/INVENTORY.md appears out of date with the repo state in this PR: the tree and folder stats omit docs/INVENTORY.md itself and tools/inventory.ps1, and the per-folder counts (e.g., docs=5, tools=10) no longer match the tracked files. Regenerate this file using tools/inventory.ps1 after all new/renamed files are in place (or consider not committing the generated output to avoid drift).
1 participant
Release v1.5.4
Highlights
Packaging / Tooling