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.

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

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Oct 17, 2024

PHPC-2649

This PR adds the vs output variable from the setup-php-sdk action to all actions involved in building Windows packages so that it can be used when determining the filename for the archive. This removes the need for maintaining a list of PHP versions with their compilers.

In addition to this, the PR makes two more adjustments for compatibility with PIE:

  • The archive name should contain x86_64 for 64-bit platforms, not x64
  • The built DLL should be named like the archive instead of the default php_mongodb.dll. PIE will rename the file in its install step.

cc @mickverm - this should get rid of the issues you encountered, but we won't know until we release 1.20.1. Please bear with us while we sort out the initial difficulties with PIE. We do appreciate your testing efforts for PIE compatibility 💪

@mickverm
Copy link
Contributor

mickverm commented Oct 17, 2024

I'll test it out as soon as a new version is released, thank you for keeping me up to date. 😄

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

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

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM assuming php_mongodb.pdb is using the correct name.

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.

@alcaeus alcaeus merged commit 4d81514 into mongodb:v1.20 Oct 18, 2024
80 checks passed
@alcaeus alcaeus deleted the phpc-2469-use-setup-php-output branch October 18, 2024 06:31
This was referenced Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants