Skip to content

PHPC-2649: Use output from setup-php-sdk in packaging step #1732

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 18, 2024
Merged

PHPC-2649: Use output from setup-php-sdk in packaging step #1732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
741767a
Use compiler name output from setup-php-sdk action in packaging step
alcaeus Oct 16, 2024
8ac30ed
Fix wrong name for x64 packages
alcaeus Oct 16, 2024
85c0317
Use correct name for DLL file to fix pie installs
alcaeus Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/actions/windows/build/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ inputs:
description: "Thread-safety (nts or ts)"
required: true
outputs:
vs:
description: "The Visual Studio version"
value: ${{ steps.prepare-build-env.outputs.vs }}
build-dir:
description: "The build directory to be used"
value: ${{ steps.prepare-build-env.outputs.build-dir }}
Expand Down
3 changes: 3 additions & 0 deletions .github/actions/windows/prepare-build/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ inputs:
description: "Thread-safety (nts or ts)"
required: true
outputs:
vs:
description: "The Visual Studio version"
value: ${{steps.setup-php.outputs.vs}}
build-dir:
description: "The build directory to be used"
value: ${{steps.get-build-dir.outputs.build_dir}}
Expand Down
35 changes: 14 additions & 21 deletions .github/workflows/build-windows-package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ jobs:
defaults:
run:
shell: cmd
outputs:
vs: ${{ steps.build-driver.outputs.vs }}

steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -110,31 +112,22 @@ jobs:
with:
filenames: php_mongodb.dll

# Copy the signature file from the release asset directory to avoid directory issues in the ZIP file
- name: "Copy signature file"
run: cp ${RELEASE_ASSETS}/php_mongodb.dll.sig .
- name: "Generate file name for DLL and archive"
run:
echo FILENAME="php_mongodb-${{ inputs.version }}-${{ inputs.php }}-${{ inputs.ts }}-${{ needs.build.outputs.vs }}-${{ inputs.arch == 'x64' && 'x64_86' || inputs.arch }}" >> "$GITHUB_ENV"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted that this uses the needs context.


- name: "Set compiler environment variable"
# In this step, we:
# - update the extension DLL file name to match the expectation of pie
# - copy the signature file from the release asset directory to avoid directory issues in the archive
# - rename the signature file to match the extension DLL file
- name: "Copy signature file and use correct file names"
run: |
case "$PHP_VERSION" in
"7.4")
COMPILER="vc15"
;;
"8.0" | "8.1" | "8.2" | "8.3")
COMPILER="vs16"
;;
"8.4")
COMPILER="vs17"
;;
esac
echo "COMPILER=${COMPILER}" >> "$GITHUB_ENV"
shell: bash
env:
PHP_VERSION: ${{ inputs.php }}
mv php_mongodb.dll ${{ env.FILENAME }}.dll
cp ${RELEASE_ASSETS}/php_mongodb.dll.sig ${{ env.FILENAME }}.dll.sig

- name: "Create and upload release asset"
if: ${{ inputs.upload_release_asset }}
run: |
ARCHIVE=php_mongodb-${{ inputs.version }}-${{ inputs.php }}-${{ inputs.ts }}-${{ env.COMPILER }}-${{ inputs.arch }}.zip
zip ${ARCHIVE} php_mongodb.dll php_mongodb.dll.sig php_mongodb.pdb CREDITS CONTRIBUTING.md LICENSE README.md THIRD_PARTY_NOTICES
ARCHIVE=${{ env.FILENAME }}.zip
zip ${ARCHIVE} ${{ env.FILENAME }}.dll ${{ env.FILENAME }}.dll.sig php_mongodb.pdb CREDITS CONTRIBUTING.md LICENSE README.md THIRD_PARTY_NOTICES
Copy link
Contributor

@mickverm mickverm Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, php_mongodb.pdb needs to become ${{ env.FILENAME }}.pdb.

php_mongodb-1.20.1-8.3-ts-vs16-x86_64.dll would be converted to php_mongodb-1.20.1-8.3-ts-vs16-x86_64.pdb in https://github.com/php/pie/blob/eaf77ea181f44c7711887474c4f33501a17cb39f/src/Installing/WindowsInstall.php#L39

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, according to the maintainer docs, it doesn't:

php_{extension-name}.pdb - this will be moved alongside the C:\path\to\php\ext\php_{extension-name}.dll

Looking at the code you linked, I do agree that it should be named the same way as the DLL file.

Let me try summoning @asgrim to clarify 😉

Another concern that comes to mind is that we're now requiring people that install their extensions manually. Right now, they only need to copy php_mongodb.dll to their extensions directory and they're done. This is valid regardless of whether they download our packages or the ones offered on pecl.php.net.

I'm sure there was a particular reason for naming the DLL file this way, but I wonder if that's truly necessary. Clearly, we can extract the relevant information (compiler, thread safety, PHP version, architecture) from the archive filename before we extract the DLL file, so we shouldn't need to re-verify this when we have the DLL file. If a user packs a DLL file into an archive for a different platform, that's their problem, not PIE's.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the first part, yes the implementation is incorrect, it was always meant to be php_{extension-name}.pdb - I've reported issue php/pie#71 and I will look into getting this fixed.

The second part; the naming was agreed since it aligns with what php/php-windows-builder action produces

gh release upload ${{ inputs.version }} ${ARCHIVE}