Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix Windows release asset handling by standardizing the Windows ZIP asset name and updating both install scripts and CI to use/upload that standardized asset, ensuring the “latest” Windows download URL is stable.
Changes:
- Update
install.shandinstall.ps1to download a fixed Windows asset name (frankenphp-windows-x86_64.zip) instead of querying the GitHub Releases API. - Adjust the Windows GitHub Actions workflow to build/upload a consistently named Windows ZIP asset and to better determine the release tag for uploads.
- Update the docs deployment workflow triggers to run when the install scripts change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| install.sh | Hard-codes Windows ZIP asset download URL for install-on-Windows paths. |
| install.ps1 | Removes GitHub API lookup; directly downloads the standardized Windows ZIP asset. |
| .github/workflows/windows.yaml | Standardizes Windows release asset naming and modifies release tag/version selection + upload conditions. |
| .github/workflows/docs.yaml | Triggers docs deployment when install scripts change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/windows.yaml
Outdated
| } elseif ($env:GITHUB_EVENT_NAME -eq "schedule") { | ||
| $frankenphpVersion = $env:GITHUB_REF | ||
| } elseif ($releaseTag) { | ||
| $frankenphpVersion = $releaseTag |
There was a problem hiding this comment.
$frankenphpVersion is set to $releaseTag for schedule / workflow_dispatch, but gh release view typically returns tags like vX.Y.Z. That means FRANKENPHP_VERSION may include a leading v, which causes the Embed Windows icon and metadata step regex (^(\d+)\.(\d+)\.(\d+)$) to not match (falling back to 0.0.0), and also changes the -DFRANKENPHP_VERSION=... macro value. Consider normalizing $releaseTag (e.g., trim a leading v) when computing FRANKENPHP_VERSION, while keeping RELEASE_TAG as the full tag for gh release upload.
| $frankenphpVersion = $releaseTag | |
| if ($releaseTag.Length -ge 2 -and $releaseTag.StartsWith("v")) { | |
| $frankenphpVersion = $releaseTag.Substring(1) | |
| } else { | |
| $frankenphpVersion = $releaseTag | |
| } |
| permissions: | ||
| contents: read | ||
| contents: write | ||
|
|
There was a problem hiding this comment.
This workflow now requests permissions: contents: write at the workflow level, even for pull_request runs where no release upload happens. To follow least-privilege, consider keeping workflow permissions as contents: read and moving contents: write to the build job (or only the job that needs to upload release assets), similar to other workflows in this repo that use job-level permissions for release uploads.
install.sh
Outdated
|
|
||
| TMPZIP="/tmp/frankenphp-windows-$$.zip" | ||
| curl -L --progress-bar "https://github.com/php/frankenphp/releases/latest/download/${WIN_ASSET}" -o "${TMPZIP}" | ||
| curl -L --progress-bar "https://github.com/php/frankenphp/releases/latest/download/frankenphp-windows-x86_64.zip" -o "${TMPZIP}" |
There was a problem hiding this comment.
The Windows install path now hard-codes the asset URL. To avoid silently downloading a GitHub 404/HTML page and then failing later in extraction, consider adding -f (fail on HTTP errors) and printing a clear error message if the download fails (similar to how the script handles missing tools).
| curl -L --progress-bar "https://github.com/php/frankenphp/releases/latest/download/frankenphp-windows-x86_64.zip" -o "${TMPZIP}" | |
| if ! curl -f -L --progress-bar "https://github.com/php/frankenphp/releases/latest/download/frankenphp-windows-x86_64.zip" -o "${TMPZIP}"; then | |
| echo "❗ Failed to download FrankenPHP for Windows. Please check your internet connection or download it manually from:" | |
| echo " https://github.com/php/frankenphp/releases/latest" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $releaseTag = $env:INPUT_REF | ||
| if (-not $releaseTag -and $env:GITHUB_EVENT_NAME -eq "schedule") { | ||
| $releaseTag = (gh release view --repo php/frankenphp --json tagName --jq '.tagName') | ||
| } |
There was a problem hiding this comment.
On scheduled runs you set RELEASE_TAG to the latest release tag via gh release view, which makes the later zip/upload steps run. However, the repository is still checked out at the scheduled ref (typically main), so this can build a main binary and upload/overwrite it onto the latest release tag (--clobber), producing a mismatched/corrupted release asset. Consider either (a) disabling release asset uploads on schedule, or (b) computing the release ref before checkout and checking out that tag (similar to .github/workflows/static.yaml), so the built sources match the tag being uploaded to.
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| INPUT_REF: ${{ (github.ref_type == 'tag' && github.ref_name) || (github.event_name == 'workflow_dispatch' && inputs.version) || '' }} |
There was a problem hiding this comment.
INPUT_REF includes workflow_dispatch inputs.version, and any non-empty value will set RELEASE_TAG and trigger the release asset upload. Since actions/checkout currently doesn't check out that ref/tag, a user can inadvertently upload an asset built from the selected branch/commit to the specified release tag. Suggest: only set RELEASE_TAG when github.ref_type == 'tag' (tag push) OR update checkout to use ref: inputs.version when provided.
No description provided.