Skip to content

Conversation

@roborourke
Copy link

Uses Dockerfile suggested by Joe on #161 to generate PHP binaries. I'm not sure if we should just commit the binaries here as they have to be downloaded when building the lambda anyway. We could get maybe 3 binaries in within the 512mb limit for function size.

Usage: npm run build:php -- <version>

<version> needs to be a fully qualified version number as it's used to match the PHP source tarballs on GitHub.

@roborourke roborourke requested review from joehoyle and rmccue March 21, 2022 17:46
@joehoyle
Copy link
Member

I think not committing them, and uploading them to s3 is still the way to go.

@kadamwhite
Copy link
Contributor

@joehoyle asked in Slack today what version of PHP is used here. It's whatever is in s3://hm-linter/bin, which probably isn't 7.4. If we don't merge this, how do we validate the version used there, and update it accordingly?

@roborourke
Copy link
Author

The PHP version currently available and in use is 7.1.

This addition makes it easy to generate binaries for different versions so far but needs to support a configuration option. We may be able to forgo this in favour of just updating the binary and having the config option set a runtime flag for the PHP version 🤔

@roborourke
Copy link
Author

Actually the script is still useful but we don’t necessarily need more than one binary available as long as it’s the latest.

@joehoyle
Copy link
Member

joehoyle commented Feb 9, 2023

This looks great, I also modified the script to also export lib files that are required to run the PHP binary. I'm able to verify the binary works in lambda by running:

docker run --platform linux/amd64 -it --rm --entrypoint /bin/bash -v $PWD:/var/task  public.ecr.aws/lambda/nodejs:12
php -v

This was a PHP 8.2 build. I haven't verified that phpcs all runs correctly though under 8.2 yet.

@joehoyle
Copy link
Member

joehoyle commented Feb 9, 2023

Agree that I think we just want to use the latest version of PHP, rather than supporting multiple versions; if that can work anyway.

@joehoyle
Copy link
Member

joehoyle commented Mar 2, 2023

Need to work out how to run these damn tests. I think we might have a hodgepodge of older tests that might not even work

@joehoyle
Copy link
Member

@roborourke if you have any availability any chance you could try work out how to run the tests and get this shipped? I'm a bit snowerd under atm.

@joehoyle
Copy link
Member

FYI we're still running 7.1.9 here!

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