AVRO-2843: [PHP] Copy composer setup from Apache Thrift
What is the purpose of the change
This PR copies the composer setup from https://github.com/apache/thrift which Apache successfully uses to publish the Thrift package to packagist.org (https://packagist.org/packages/apache/thrift). This is in an attempt to fix https://issues.apache.org/jira/browse/AVRO-2843
Verifying this change
This change does not add any tests, since it includes no code changes.
Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
The linting action was failing because without the monorepo plugin composer install installs the vendor directory in the root of the repo rather than inside lang/php. Updating the path in PHP's build.sh seems to fix it locally, but let's see if they also helps in the actions.
I made one additional change to lang/php/test/test_helper.php so it (like build.sh) points to the vendor directory at the repo root level rather than one inside lang/php. Locally, that allows the tests to pass, although I get this as a summary:
OK, but incomplete, skipped, or risky tests! Tests: 415, Assertions: 969, Risky: 415.
Most of the "risky" tests complain like this:
This test does not have a @covers annotation but is expected to have one
but that sees to me like it should be addressed on a separate PR to keep the scope limited.
The risky warnings come from https://github.com/apache/avro/blob/main/lang/php/phpunit.xml#L22, which we could either set to false for now, or we could add the annotations it expects. Either way, this seems to be ready :)
Although this PR does what it says on the tin (it copies the working setup from the Thrift repository) one thing @adamquaile spotted while working on this is that the exclude directive does not do what we expected it to do. That is, if you install the package with composer on a target machine, a full copy of the repository will end up in that target machine's file system. One would expect this issue to also affect Thrift.
The monorepo plugin would, one assumes, fix this problem, but if we wanted to re-instate that we would have to figure out what about the original setup was not working (and preventing eg. @adamquaile from installing from main).
@jjatria It looks like the monorepo setup doesn't work as assumed.
Just tested this:
composer init
...
composer config repositories.repo-name vcs https://github.com/apache/avro
composer require apache/avro:dev-main
After that, I can still see all the files in this repo by running:
find vendor/apache/avro/
So, I don't see a reason to keep the monorepo usage.
Thanks for doing this @jjatria, been struggling with this issue for a while now. Any idea when it will be merged?
@skystebnicki Not really. Other than my comment about the keywords to use for Avro in the composer.json file (currently set to "RPC"), I think this is ready to be merged, but the merging decision is up to the maintainers. Perhaps @martin-g has more insights?
I have no experience with these matters and I totally depend on you here. Recently @jjatria also said that he does not have much experience with PHP, so I expected someone else from the community to review and approve the changes.
@martin-g I had similar issue with monorepo, the issue is present in the newest version of composer so I think it's quite important to fix it. I checked changes from this PR and it seems to be working.
If you still afraid that monorepo plugin is important we definitely should change constraint version to "~0.17" to support new updates.
Currently this library is completely broken for new composer.
As I said earlier - I have no experience with PHP. I need two approvals for this PR by users of this library to merge it. So far there are 0 approvals!
Please open https://github.com/apache/avro/pull/3057/files and use the Review > Approve functionality.
If you still afraid that monorepo plugin is important we definitely should change constraint version to "~0.17" to support new updates.
I have no preferences! Either approve this PR or open a new one with the alternative solution!
Thank you all!
Please remind me/us to upload the next release to https://packagist.org !
BTW the Docker build (used for making releases) now fails with:
Composer is operating significantly slower than normal because you do not have the PHP curl extension enabled.
No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.
Problem 1
- Root composer.json requires PHP extension ext-curl * but it is missing from your system. Install or enable PHP's curl extension.
https://github.com/apache/avro/actions/runs/13179052484/job/36785107081
Could someone send a PR to fix it ? It also complains about a missing composer.lock file.
BTW the Docker build (used for making releases) now fails with:
Composer is operating significantly slower than normal because you do not have the PHP curl extension enabled. No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information. Loading composer repositories with package information Updating dependencies Your requirements could not be resolved to an installable set of packages. Problem 1 - Root composer.json requires PHP extension ext-curl * but it is missing from your system. Install or enable PHP's curl extension.https://github.com/apache/avro/actions/runs/13179052484/job/36785107081
Could someone send a PR to fix it ? It also complains about a missing composer.lock file.
Here's a PR that fixes it (in my testing at least)