Fix PECL publish #85

TingDaoK · 2023-02-22T22:07:49Z

ISSUE:

PECL will remove the php files from subdirectory while installing.

Fix:

Move the php script that help to generate sub to main directory
Fixed a bunch of error during prepare package for pecl
Load the native extension before calling composer
add github action to create the tar file after cut the release
restructure a bit to be more clear

Windows issue:

We have a fixed composer path from makefile. And using the makefile to create a script to run tests.
We don't have CI

Fix:

Use Makefile to get the path to the shared libs (extensions) via: generate-php-ini
Updated run_tests.bat to get the path to composer.phar and accept passing in php path to override the default php
Do the same thing we did for linux.

TODO:

update README for windows and composer, PECL and releasing fixes for composer -- Composer update 2 #89
Rewrite Scripts from shell script to python for multiple platform - Rewrite script #87
Automate release process - PECL release follow up #90
Supports for PHP 8.1/8.2
Make the native extension required for composer Maybe not do this, or aws-sdk-php will break as it requires the composer package from crt, but not the native extension. Or, aws-sdk-php should take aws-crt-php an optional dependency instead (suggest instead of require)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

graebm · 2023-02-24T17:37:33Z

.github/workflows/ci.yml

+        php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
+        php composer-setup.php


is this necessary?

ubuntu-latest comes with composer preinstalled

To have composer.phar in the same directory. So, that we don't need to script to get the path of composer.phar.

installing composer twice seems like it could have weird consequences. let's just find the path. maybe something like?:

# get path to composer.phar so we can run it with custom php.ini COMPOSER_PHAR=$(realpath $(which composer)) php -c php.ini $COMPOSER_PHAR require ...

.github/workflows/ci.yml

.github/workflows/release.yml

dev-scripts/cleanup.sh

dev-scripts/run_tests.bat

php-win.ini

dev-scripts/prepare_release.sh

dev-scripts/prepare_package_xml.sh

waahm7 · 2023-02-23T22:12:53Z

.github/workflows/ci.yml

@@ -45,6 +39,13 @@ jobs:
        ./configure
        make


Debateable: // TODO run tests (if we are officially supporting 5.5 for now)

Maybe we should ask when we can drop support for older versions of PHP. If it's something like 1-2 months away, we can drop support now and sdk-php can pick up new changes later.

Let's leave the php version related stuff as a follow up item, I have listed some TODOs in the description of the PR.

waahm7 · 2023-02-24T18:46:19Z

Makefile.frag

+	# install awscrt.stub.php to ext/
+	cp -v awscrt.stub.php ext/awscrt.stub.php


Why? We moved this to the main folder instead of ext/ and making a copy here.

PECL removes .php from subdirectory, but not from the root directory. So, move it outside the ext/ is the easiest way.

Why do we need this in ext/? I am just confused about why do we need this file at two places.

# generate awscrt_arginfo.h php gen_stub.php --minimal-arginfo ext/awscrt.stub.php

This part will generate the awscrt_arginfo.h into the same folder as awscrt.stub.php.
The gen_sub.php is from the PHP build scripts, I didn't dig deep into it. We need awscrt_arginfo.h in ext/, so I just copy it around here.

waahm7 · 2023-02-24T19:08:40Z

.github/workflows/ci.yml

+      uses: cmb69/setup-php-sdk@v0.6
+      with:
+        version: '8.0'
+        arch: x64
+        ts: ts
+        deps: openssl


Confused: What is this action? I could not find anything about it using a quick Google search (https://github.com/marketplace?type=actions&query=Setup+PHP+). Can you please add some documentation for why we need a different action for Windows CI? It appears that shivammathur/setup-php@v2 also supports Windows.

TBH, I didn't dig deep. I followed the PR initialed from Mike, https://github.com/awslabs/aws-crt-php/pull/77/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR114.
And I believe Mike found it from https://github.com/Imagick/imagick/blob/master/.github/workflows/windows.yml

I was really just want to have something working...

waahm7 · 2023-02-24T19:09:57Z

.github/workflows/ci.yml

+        shell: cmd # use CMD instead of powershell to catch error from bat script
+    strategy:
+      matrix:
+        arch: [x64]


Confused: Why only x64? Is this a todo for 32bit?

Yeah, maybe should leave it as a TODO. Again, just being lazy as no current ask and it's probably work.

.github/workflows/ci.yml

composer.json

This reverts commit b116a18.

…-php into native-extension-rewrite

graebm

fix & ship

graebm · 2023-02-28T18:09:24Z

.github/workflows/ci.yml

+        php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
+        php composer-setup.php


installing composer twice seems like it could have weird consequences. let's just find the path. maybe something like?:

# get path to composer.phar so we can run it with custom php.ini COMPOSER_PHAR=$(realpath $(which composer)) php -c php.ini $COMPOSER_PHAR require ...

.github/workflows/ci.yml

dev-scripts/cleanup_build.py

TingDaoK added 30 commits February 21, 2023 14:28

require composer to load awscrt extension

e5e998a

load extension before run composer

31a5785

make test after the composer nonsense

82abc2d

try to just use make test?

1a0efbf

run test

17b6a93

clean up

9ff650e

restructure

9de07ef

change dir from run test

826974a

update the script path from ci

f6314a2

create release action when we cut a new release

1f6f814

more fix

9de1d33

make a github action to test pecl before release

f183523

why not find

093cf4e

use mac

b6a3a29

skip verification

08aa34b

build and test as well

3015509

just make sure it builds

279f5d2

use mac to release

a7e8407

trivial

02a6591

oh, it was a typo..

7d56d18

windows CI

26ab397

get the right directory

8208b31

why?

311edc6

this?

2aae069

why cannot find dll

8455b69

one more test

babc4bf

fine

3ad39d5

generate php-win.ini

72c24d9

get the right php

7c12d40

give me echo

a28388f

TingDaoK and others added 3 commits February 23, 2023 17:14

I don't know

ce4a00c

I don't know

8c66121

Windows ci fix (#86)

e361f03

graebm reviewed Feb 24, 2023

View reviewed changes

waahm7 reviewed Feb 24, 2023

View reviewed changes

TingDaoK and others added 5 commits February 27, 2023 13:58

it should not be needed for the config platform

b116a18

Revert "it should not be needed for the config platform"

c0ab711

This reverts commit b116a18.

remove release.yml and leave it as todo to follow up quick

c2f584a

Rewrite script (#87)

ff20f95

Merge branch 'native-extension-rewrite' of github.com:awslabs/aws-crt…

b4d352c

…-php into native-extension-rewrite

graebm approved these changes Feb 28, 2023

View reviewed changes

TingDaoK added 3 commits February 28, 2023 10:53

try CMD to run multiple lines of code

a63ac28

try to not force the special version of phpunit

a9ab031

update comments

6bc427d

waahm7 approved these changes Feb 28, 2023

View reviewed changes

stobrien89 approved these changes Mar 1, 2023

View reviewed changes

TingDaoK merged commit 442f385 into main Mar 1, 2023

TingDaoK deleted the native-extension-rewrite branch March 1, 2023 22:56

TingDaoK mentioned this pull request Mar 9, 2023

Win x86 #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PECL publish #85

Fix PECL publish #85

TingDaoK commented Feb 22, 2023 •

edited

graebm Feb 24, 2023

TingDaoK Feb 24, 2023

graebm Feb 28, 2023

waahm7 Feb 23, 2023

waahm7 Feb 24, 2023

TingDaoK Feb 27, 2023

waahm7 Feb 24, 2023

TingDaoK Feb 24, 2023

waahm7 Feb 24, 2023

TingDaoK Feb 24, 2023

waahm7 Feb 24, 2023

TingDaoK Feb 27, 2023

waahm7 Feb 24, 2023

TingDaoK Feb 27, 2023

graebm left a comment

graebm Feb 28, 2023

		php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
		php composer-setup.php

		# install awscrt.stub.php to ext/
		cp -v awscrt.stub.php ext/awscrt.stub.php

Fix PECL publish #85

Fix PECL publish #85

Conversation

TingDaoK commented Feb 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graebm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingDaoK commented Feb 22, 2023 •

edited