Skip to content
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

Build Linux packages for the extension #107

Closed

Conversation

SergeyKleyman
Copy link

Copy link

linux-foundation-easycla bot commented Nov 2, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Collaborator

@brettmc brettmc left a comment

Choose a reason for hiding this comment

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

This looks great! A few comments and observations from me. Do you suggest that we should run this build as part of each push, and also do the publish when we create a release? If so, we will also need some actions added under .github

#### Function php_ini_file_path ################################################
function php_ini_file_path() {
php_command -i \
| grep 'Configuration File (php.ini) Path =>' \
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this be replaced by php-config --ini-path ?


################################################################################
#### Function php_config_d_path ################################################
function php_config_d_path() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

php-config --ini-dir ?


echo "Uninstalling ${OPENTELEMETRY_INI_FILE_NAME} for supported SAPI's"

# Detect installed SAPI's
Copy link
Collaborator

Choose a reason for hiding this comment

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

php-config --php-sapis ?

packaging/create-package.sh Outdated Show resolved Hide resolved
BUILD_EXT_DIR=instrumentation/native/_build/linux-x86-64-release/ext/
createPackage

NAME="${NAME_BACKUP}-debugsymbols-linux-x86-64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, but I think just debug would be ok here?

packaging/post-install.sh Outdated Show resolved Hide resolved
PHP_INSTRUMENTATION_DIR=/opt/open-telemtry/php-instrumentation
EXTENSION_DIR="${PHP_INSTRUMENTATION_DIR}/extensions"
EXTENSION_CFG_DIR="${PHP_INSTRUMENTATION_DIR}/etc"
BOOTSTRAP_FILE_PATH="${PHP_INSTRUMENTATION_DIR}/src/bootstrap_php_part.php"
Copy link
Collaborator

@brettmc brettmc Nov 3, 2023

Choose a reason for hiding this comment

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

I think BOOTSTRAP_FILE_PATH is vestigial from elastic apm, and not needed here

################################################################################
#### Function php_api ##########################################################
function php_api() {
php_command -i \
Copy link
Collaborator

Choose a reason for hiding this comment

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

if there was a php-config --api, we could get everything more effectively from there...

dont clobber the dockerfile arg for PHP_CONFIG_OPTS, which includes --with-pear. this will allow us to pear package within the build
@brettmc
Copy link
Collaborator

brettmc commented Nov 14, 2023

Hopefully this will integrate nicely with #109 when it's merged - we can publish these packages as build artifacts for each github actions run, and also as release artifacts.

brettmc and others added 23 commits November 16, 2023 00:41
test that post hooks still execute after die/exit is called
…lemetry#109)

we already build windows binaries, so let's make them available to users. On each github action, the binaries are published as build artifacts.
When a tag is created, we now create a draft release from the workflow (instead of manually creating the release via the GUI), which means we can also upload the binaries as release artifacts.
Give PECL package the same treatment, so that maintainers don't have to build it manually and upload.
After this is merged, the release process for the extension will change slightly: it relies on a tag being created (but not a release). You can't do that from the GUI, so there's a change coming to dev-tools which will create the tag, and from there the workflow will take care of creating the release in draft form. The final check is for a maintainer to eyeball the release, make any modifications to the text, and publish it.
adds 8.3 local builds, and also github build of our debug/dev image
* Include PHP 8.3 in build matrix

* Bump deps in actions

* Upgrade default php version

* Switch to php/setup-php-sdk

* Keep actions up to date

---------

Co-authored-by: Brett McBride <[email protected]>
* Isolate exception state for hooks
* Consistent naming.
* Fix formatting and test typo
* Fixed test list in PECL package.xml
* Save and restore execute_data->opline
* adding test for post hook type error
if an invalid typehint is provided for first param of post callback, this would previously cause a hang.
fixed by open-telemetry#118, this test confirms the fix works in this scenario.

* improve test, add to package.xml

* remove try/catch
* Support modifying named params

* Removed use of function not available in all PHP versions
…n-telemetry#122)

* Unregister INI in MSHUTDOWN instead of RSHUTDOWN

* Added test

* Fix test name typo
…etry#120)

* Fix modifying extra args, limit argument expansion

* Added some comments and one more test

* Fix func_get_args crash on excess internal func params

* Fix typo in comment
- warning: unused variable '...' [-Wunused-variable]
- warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'zend_ulong' {aka 'long unsigned int'} [-Wformat=]
* clang format

* update checkout action to v4
…hook (open-telemetry#127)

* Use E_CORE_WARNING for logging

* Add test, fix other tests

---------

Co-authored-by: Brett McBride <[email protected]>
@bobstrecansky
Copy link
Collaborator

Going to close this for now; please reopen if you're interested in finishing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants