Build script fixed + enhanced #4000
Conversation
Main issues: redundant 95MB file removed from portable zip; fixed plugin build logic so that we always get the latest version and not an obsolete DLL.
---
1. **Fixed Portable Package Creation**
* The portable build process was including the temporary `packages` directory in the final archive. This added `FlowLauncher-x.x.x-full.nupkg` file, increasing zip size by ~95MB. We now explicitly remove this directory before compression.
* We now use a staging directory to assemble the portable version by running a silent install which ensures package structure is correct and contains no unnecessary build artifacts.
2. **Enhanced Build Process Reliability**
* **Dynamic Squirrel Path:** The script no longer uses a hardcoded path to `squirrel.windows`.
* **Improved Dependency Handling:** By changing `<Private>` from `false` to `true` in `Directory.Build.targets`, project-to-project dependencies are now correctly copied to the output directory. This prevents "missing assembly" errors, especially for plugins.
* **Safer File Deletion:** The `Remove-UnusedFiles` function now includes a safety check to prevent accidentally deleting a plugin's main DLL if it shares a name with a common dependency.
* **Structured Build Flow:** The main build logic has been restructured to follow a sequence of building the solution, publishing to a temporary directory, and then merging into the final release folder, which prevents interference from old build artifacts.
* **More Reliable Version Retrieval:** The `Build-Version` function was updated to use `(Get-Item).VersionInfo.ProductVersion` instead of `Get-Command`. This is more reliable for retrieving file version in PowerShell and aligns with `AssemblyInformationalVersion`.
3. **Modernized NuGet Package**
* The `.nuspec` file has been updated to use the modern `<icon>` and `<readme>` tags instead of the deprecated `<iconUrl>`.
|
🥷 Code experts: jjw24 jjw24 has most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
We have tried to move to Velopack + Github Action in #2616 |
|
Before we get too deep into enhancing squirrel.windows I actually would like to move to Velopack if possible, it provides build versioning and ability to use pre-release channel. |
Is this safe to do? Been a long while I looked at squirrel, is this file needed for updating purposes? |
📝 WalkthroughWalkthroughSwitches two MSBuild ProjectReference Private flags to true. Updates the NuGet .nuspec to include icon, readme, framework group, and extra files. Refactors post_build.ps1 with renamed functions, new staging/merge steps, updated version retrieval, stricter Squirrel packaging, and a revised end-to-end build/publish flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant PS as post_build.ps1
participant MSB as Build-Solution
participant PubTmp as Publish-SelfContainedToTemp
participant Merge as Merge-PublishToRelease
participant Ver as Build-Version
participant Res as Copy-Resources
participant Clean as Remove-UnusedFiles
participant Dump as Remove-CreateDumpExe
participant Sq as New-SquirrelInstallerPackage
participant Port as Publish-Portable
Dev->>PS: Invoke script (path, config)
PS->>MSB: Build-Solution(p)
MSB-->>PS: build output
PS->>PubTmp: Publish-SelfContainedToTemp(p, tempPath)
PubTmp-->>PS: temp publish
PS->>Merge: Merge-PublishToRelease(tempPath, releasePath)
Merge-->>PS: release merged
PS->>Ver: Build-Version(releasePath)
Ver-->>PS: ProductVersion
alt version not found
PS-->>Dev: Abort with error
end
PS->>Res: Copy-Resources(releasePath)
Res-->>PS: resources copied (Squirrel.exe resolved if present)
PS->>Clean: Remove-UnusedFiles(releasePath, config)
Clean-->>PS: pruned
PS->>Dump: Remove-CreateDumpExe(releasePath)
Dump-->>PS: cleaned
PS->>Sq: New-SquirrelInstallerPackage(releasePath, version, output)
alt Squirrel.exe missing or non-zero exit
Sq-->>PS: error
PS-->>Dev: Abort with error
else success
Sq-->>PS: installer created
PS->>Port: Publish-Portable(outputLocation, version, releasePath)
Port-->>PS: portable package
PS-->>Dev: Done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (8)
Scripts/flowlauncher.nuspec (1)
20-21: Confirm file paths resolve with your current pack base path.
- src paths are relative to the .nuspec. If the nuspec sits in Scripts/, “images\app.png” and “....\README.md” must exist relative to it when nuget pack runs. (github.com)
Scripts/post_build.ps1 (7)
36-41: Copying Squirrel.exe as Update.exe is acceptable; choose destination intentionally.Renaming Squirrel.exe to Update.exe is a documented workaround for dev scenarios. Consider copying into Output\Release (or drop entirely if unused by packing) to avoid stray artifacts in Output.
Reference. (github.com)
48-54: Guard to avoid deleting a plugin’s main DLL may be brittle.Many Flow plugins’ main DLL name doesn’t equal the parent directory. The existing version‑match filter likely prevents false positives, but please validate against a few plugins (System, Everything, etc.).
84-90: Good: decouple from hardcoded Squirrel path; add exit‑code checks and avoid aliasing.
- After nuget pack and Squirrel --releasify, check $LASTEXITCODE and throw/return false.
- Prefer invoking & $squirrelExe directly over New-Alias to avoid scope surprises.
Apply this diff:
- nuget pack $spec -Version $version -BasePath $inputPath -OutputDirectory $output -Properties Configuration=Release + nuget pack $spec -Version $version -BasePath $inputPath -OutputDirectory $output -Properties Configuration=Release + if ($LASTEXITCODE -ne 0) { throw "nuget pack failed with exit code $LASTEXITCODE" } @@ - New-Alias Squirrel $squirrelExe -Force + # Invoke Squirrel.exe directly to avoid aliasing @@ - Squirrel --releasify $nupkg --releaseDir $temp --setupIcon $icon --no-msi | Write-Output + & $squirrelExe --releasify $nupkg --releaseDir $temp --setupIcon $icon --no-msi | Write-Output + if ($LASTEXITCODE -ne 0) { throw "Squirrel releasify failed with exit code $LASTEXITCODE" }Also applies to: 96-104
125-135: Publish step is fine; consider returning errors explicitly.Returning $false already handled upstream; OK.
137-141: OK to merge then delete temp; consider robocopy for large trees.Copy-Item can be slow; not blocking.
177-181: Answering the PR question: removing “packages” (thus the large full.nupkg) from the portable zip is OK.Squirrel’s updater relies on a remote RELEASES feed (with -full/-delta .nupkg). Shipping the local “packages” cache in a portable zip is not required; Update.exe will recreate it during the first update.
To be extra‑sure, test: extract the portable zip, run Update.exe --update , and confirm it fetches the next version and recreates “packages”. References. (gist.github.com)
84-104: Future: consider Velopack migration.Velopack (Squirrel successor) simplifies update feeds and uses a 'current' junction instead of app-, which can reduce path/version coupling in scripts. Worth exploring in a separate PR.
Reference on app folder behavior. (github.com)
Also applies to: 143-198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Directory.Build.targets(1 hunks)Scripts/flowlauncher.nuspec(1 hunks)Scripts/post_build.ps1(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-08T15:52:58.573Z
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Applied to files:
Scripts/flowlauncher.nuspecScripts/post_build.ps1
📚 Learning: 2024-06-19T18:57:05.138Z
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Scripts/post_build.ps1:2-5
Timestamp: 2024-06-19T18:57:05.138Z
Learning: The `$flowVersion` parameter in the `post_build.ps1` script is intentionally left unset for dynamic assignment during script execution. The script contains logic to handle cases where this parameter remains unset.
Applied to files:
Scripts/post_build.ps1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (5)
Directory.Build.targets (1)
8-12: Switching ProjectReference Private to true is sensible; verify no unexpected duplication in plugin outputs.This should prevent missing-assembly issues at runtime, especially for plugins. Please sanity‑check that this doesn’t bloat plugin folders or conflict with plugin-local copies during packaging.
Scripts/flowlauncher.nuspec (1)
9-16: Icon/readme metadata updates look good; keep iconUrl for Squirrel, but rely on icon for NuGet.NuGet recommends icon over iconUrl; keeping both maintains back‑compat and Squirrel friendliness. Empty net9.0 dependency group is fine but optional. Confirm these assets resolve correctly at pack time.
Relevant docs: icon/iconUrl/readme fields and file inclusion behavior. (learn.microsoft.com)
Scripts/post_build.ps1 (3)
71-76: LGTM: idempotent directory init.
117-123: dotnet build gate looks fine.
205-238: Main flow sequencing looks good; add early exits on failures already covered.After the Squirrel packaging we already abort on failure; good. No further blockers once the fixes above land.
| # Use Get-Item for reliability and ProductVersion to align with AssemblyInformationalVersion. | ||
| $v = (Get-Item $targetPath).VersionInfo.ProductVersion | ||
| } else { |
There was a problem hiding this comment.
Normalize ProductVersion to a Squirrel/NuGet‑safe version (strip +build metadata).
Build metadata (e.g., 1.2.3-beta+abc) can break folder matching (app-$version) and sometimes NuGet/Squirrel expectations. Strip anything after “+”.
Apply this diff:
- $v = (Get-Item $targetPath).VersionInfo.ProductVersion
+ $v = (Get-Item $targetPath).VersionInfo.ProductVersion
+ # Normalize for NuGet/Squirrel: remove +build metadata to match app-<version> folder naming.
+ $v = $v -replace '\+.*$', ''Evidence that Squirrel uses app- conventions and Release feed versions. (github.com)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Use Get-Item for reliability and ProductVersion to align with AssemblyInformationalVersion. | |
| $v = (Get-Item $targetPath).VersionInfo.ProductVersion | |
| } else { | |
| # Use Get-Item for reliability and ProductVersion to align with AssemblyInformationalVersion. | |
| $v = (Get-Item $targetPath).VersionInfo.ProductVersion | |
| # Normalize for NuGet/Squirrel: remove +build metadata to match app-<version> folder naming. | |
| $v = $v -replace '\+.*$', '' | |
| } else { |
🤖 Prompt for AI Agents
In Scripts/post_build.ps1 around lines 10 to 12, the ProductVersion from
Get-Item may include +build metadata (e.g. 1.2.3-beta+abc) which breaks
Squirrel/NuGet folder/version conventions; after assigning $v from
VersionInfo.ProductVersion, normalize it by stripping any '+' and all following
characters (so only the semver+prerelease part remains) and reassign the cleaned
value to $v before it is used for folder naming or packaging.
| $depjsonPath = (Get-Item "$target\Flow.Launcher.deps.json").FullName | ||
| if (Test-Path $depjsonPath) { | ||
| $depjson = Get-Content $depjsonPath -raw | ||
| $depjson -replace '(?s)(.createdump.exe": {.*?}.*?\n)\s*', "" | Out-File $depjsonPath -Encoding UTF8 | ||
| } | ||
| Remove-Item -Path $target -Include "*createdump.exe" -Recurse |
There was a problem hiding this comment.
Fix: Get-Item runs before the existence check and will throw if the file is missing.
Swap to Test-Path before Get-Item.
Apply this diff:
- $depjsonPath = (Get-Item "$target\Flow.Launcher.deps.json").FullName
- if (Test-Path $depjsonPath) {
- $depjson = Get-Content $depjsonPath -raw
+ $depjsonPath = Join-Path $target "Flow.Launcher.deps.json"
+ if (Test-Path $depjsonPath) {
+ $depjson = Get-Content $depjsonPath -Raw
$depjson -replace '(?s)(.createdump.exe": {.*?}.*?\n)\s*', "" | Out-File $depjsonPath -Encoding UTF8
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $depjsonPath = (Get-Item "$target\Flow.Launcher.deps.json").FullName | |
| if (Test-Path $depjsonPath) { | |
| $depjson = Get-Content $depjsonPath -raw | |
| $depjson -replace '(?s)(.createdump.exe": {.*?}.*?\n)\s*', "" | Out-File $depjsonPath -Encoding UTF8 | |
| } | |
| Remove-Item -Path $target -Include "*createdump.exe" -Recurse | |
| $depjsonPath = Join-Path $target "Flow.Launcher.deps.json" | |
| if (Test-Path $depjsonPath) { | |
| $depjson = Get-Content $depjsonPath -Raw | |
| $depjson -replace '(?s)(.createdump.exe": {.*?}.*?\n)\s*', "" | Out-File $depjsonPath -Encoding UTF8 | |
| } | |
| Remove-Item -Path $target -Include "*createdump.exe" -Recurse |
🤖 Prompt for AI Agents
In Scripts/post_build.ps1 around lines 62 to 67 the script calls Get-Item on
"$target\Flow.Launcher.deps.json" before checking the file exists, which will
throw if the file is missing; change the order to build the path string first,
use Test-Path on that path and only then call Get-Item to obtain FullName (or
directly operate on the path string), then continue with the content
read/replace and removal steps.
| # The portable version is created by silently running the installer to a temporary location, | ||
| # then packaging the result. This ensures the structure is identical to a real installation | ||
| # and can be updated by Squirrel. | ||
| & "$outputLocation\Flow-Launcher-Setup.exe" --silent | Out-Null | ||
|
|
||
| $installRoot = Join-Path $env:LocalAppData "FlowLauncher" | ||
| $appPath = Join-Path $installRoot "app-$version" | ||
| $appExePath = Join-Path $appPath "Flow.Launcher.exe" | ||
|
|
||
| # Wait for silent installation to complete | ||
| $waitTime = 0 | ||
| $maxWaitTime = 60 # 60 seconds timeout | ||
| while (-not (Test-Path $appExePath) -and $waitTime -lt $maxWaitTime) { | ||
| Start-Sleep -Seconds 1 | ||
| $waitTime++ | ||
| } | ||
|
|
||
| if (-not (Test-Path $appExePath)) { | ||
| Write-Host "Error: Timed out waiting for silent installation to complete." -ForegroundColor Red | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Portable packaging flow: two risks and one hardening suggestion.
- Risk 1: Silent install to %LocalAppData%\FlowLauncher will overwrite an existing user install on dev machines, then uninstall it later. Gate this behind CI env checks or install to an isolated temp root.
- Risk 2: Waiting 60s may be flaky on slow agents; increase timeout or wait on the spawned process.
- Hardening: Don’t rely on $version for app- path; pick the newest app-* folder to tolerate version normalization differences.
Apply this diff:
- & "$outputLocation\Flow-Launcher-Setup.exe" --silent | Out-Null
+ # Avoid clobbering a developer's local install; allow override via env var
+ if (-not $env:CI -and -not $env:APPVEYOR) {
+ Write-Host "Warning: Skipping silent install outside CI to avoid overwriting a local install." -ForegroundColor Yellow
+ return
+ }
+ & "$outputLocation\Flow-Launcher-Setup.exe" --silent | Out-Null
@@
- $appPath = Join-Path $installRoot "app-$version"
- $appExePath = Join-Path $appPath "Flow.Launcher.exe"
+ # Prefer discovering the latest app-* folder to avoid version string mismatches
+ $latestAppDir = Get-ChildItem -Path $installRoot -Directory -Filter "app-*" | Sort-Object LastWriteTime -Descending | Select-Object -First 1
+ $appPath = $latestAppDir?.FullName
+ $appExePath = if ($appPath) { Join-Path $appPath "Flow.Launcher.exe" } else { $null }
@@
- $maxWaitTime = 60 # 60 seconds timeout
+ $maxWaitTime = 180 # allow slower agents
@@
- Write-Host "Error: Timed out waiting for silent installation to complete." -ForegroundColor Red
+ Write-Host "Error: Timed out waiting for silent installation to complete." -ForegroundColor Red
returnOn removing “packages” from the portable zip: this is safe for update functionality; Update.exe downloads RELEASES and nupkg files from the configured feed on demand, and the local packages cache is recreated as needed. (gist.github.com)
Also applies to: 167-185, 186-198
🤖 Prompt for AI Agents
In Scripts/post_build.ps1 around lines 144-165 (also apply same changes to lines
167-185 and 186-198), the portable packaging flow risks overwriting a
developer's existing install, can time out waiting for a silent install, and
brittlely assumes app-$version exists; fix by: 1) gate the silent installer run
so it only executes in CI (e.g., check for a CI-specific env var) or change the
install target to an isolated temporary install root under $env:TEMP instead of
$env:LocalAppData\FlowLauncher; 2) stop using a fixed 60s loop—either capture
the installer process and Wait-Process for it or increase the timeout and
backoff retries to be robust on slow agents; 3) replace the hardcoded Join-Path
$installRoot "app-$version" logic with code that enumerates app-* directories
under the install root and selects the newest (highest semver or newest
LastWriteTime) folder to find Flow.Launcher.exe; also remove local “packages”
from the portable zip packaging per the suggested change so updates rely on the
feed.
Thanks. It's not clear from that why it was closed??
That makes the most sense for sure. Lots of advantages.
I don't think it is but have done minimal testing (installer + portable works), I'm not familiar with Squirrel and was going to rely on you guys to review the PR / help test. But if Velopack is the preferred solution then I'm happy to have a go, and let this PR sit for now? |
|
I have no idea about why #2616 is closed. And I would prefer Velopack. If you have similar ideas, I think we can put this PR and let us go with Velopack. |
OK, I will make a start after I finish the new BrowserBookmarks plugin, which I'm looking at now. |
Main issues: redundant 95MB file removed from portable zip; fixed plugin build logic so that we always get the latest version and not an obsolete DLL.
Fixed Portable Package Creation
packagesdirectory in the final archive. This addedFlowLauncher-x.x.x-full.nupkgfile, increasing zip size by ~95MB. We now explicitly remove this directory before compression.Enhanced Build Process Reliability
squirrel.windows.<Private>fromfalsetotrueinDirectory.Build.targets, project-to-project dependencies are now correctly copied to the output directory. This prevents "missing assembly" errors, especially for plugins.Remove-UnusedFilesfunction now includes a safety check to prevent accidentally deleting a plugin's main DLL if it shares a name with a common dependency.Build-Versionfunction was updated to use(Get-Item).VersionInfo.ProductVersioninstead ofGet-Command. This is more reliable for retrieving file version in PowerShell and aligns withAssemblyInformationalVersion.Modernized NuGet Package
.nuspecfile has been updated to use the modern<icon>and<readme>tags (note - I failed to get rid of the deprecated<iconUrl>due to Squirrel).Note
I've been playing with https://github.com/velopack/velopack for https://github.com/dcog989/Cliptoo - would there be interest in converting Flow to use this given that Squirrel was abandoned long ago? (Velopack originates from a fork of Squirrel).