GH Action: Add steps to test against libxml >= 2.12 #798

asispts · 2025-01-25T13:59:37Z

Description

Follow up of #797.

I've update GH action to run tests against libxml >= 2.12. Also, I added fail-fast: false to ensure that other matrix jobs continue running until completion.

Note

Since libxml >= 2.12 is not available in ubuntu-latest, the library is compiled from source.

Suggested changelog entry

Related issues/external references

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
- This change is only breaking for integrators, not for external standards or end-users.
Documentation improvement

PR checklist

I have checked there is no other PR open for the same change.
I have read the Contribution Guidelines.
I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.
[Required for new sniffs] I have added XML documentation for the sniff.

jrfnl · 2025-01-25T18:02:08Z

@asispts Would you mind rebasing this PR ? as it needs the changes from #797 (now merged) to allow the build to pass.

asispts · 2025-01-26T03:32:15Z

@jrfnl done

jrfnl

Thank you for working on this @asispts ! I really appreciate your help in stabilizing this part of the test suite and safeguarding the functionality in different circumstances.

I do have some questions and remarks, which I've left in line. Questions do not necessarily imply something needs to change. In most cases, I'd just like to understand the reasoning why.

.github/workflows/test.yml

jrfnl · 2025-01-26T12:20:37Z

.github/workflows/test.yml

@@ -93,6 +96,20 @@ jobs:
      - name: Checkout code
        uses: actions/checkout@v4

+      # A temporary solution
+      - name: Install libxml2 >= 2.12 (PHP 8.1+ on linux only)
+        if: ${{ matrix.os == 'ubuntu-latest' && contains(fromJSON('["8.1", "8.2", "8.3"]'), matrix.php) }}


I have a number of question about this:

Does this have to be tested on multiple PHP versions or would testing on a single PHP 8.1+ version be sufficient ? (the compile from scratch slows the build down with ~45-60 second extra build time)

How was it decided to test against PHP 8.1, 8.2 and 8.3, but not against PHP 8.4 or 8.5 ?

As for the usability/comprehension of failures if/when they would occur:
As things are, if a build using libxml 2.12 would fail, it is not transparent for a non-regular contributor that libxml 2.12 is being used and may be the cause of the failure. They would have to study the workflow to figure that out.

Did you consider making the use of libxml 2.12 a matrix flag ? And if you considered this, what was the reason to decide against that ?
Note: if it would be a matrix flag, that would also allow for annotating that libxml 2.12 is being used for a certain job in the job name.

Last question: not sure if that would work, but did you consider/have you tried installing libxml 2.12 via the extensions key in the setup-php action runner ?
Ref: https://github.com/shivammathur/setup-php?tab=readme-ov-file#heavy_plus_sign-php-extension-support

@jrfnl The issue is related to libxml2 library, not the PHP libxml extension.

I've added a comment to explain why is this a temporary solution. Also, I used homebrew, as suggested by @derrabus, to install latest libxml2 version and remove the hardcoded version.

@asispts Thanks for your continued work on this.

While my last question (third comment here) has been answered, the questions posed in my first two comments have not yet been answered and I'd still be interested to understand the reasoning for the current choices.

.github/workflows/test.yml

derrabus · 2025-01-26T17:09:02Z

Compiling libxml2 on each build sounds expensive. Would it be faster to use Homebrew for the installation? Also, do we need to test with every PHP version? Wouldn't it be enough if we installed the custom libxml2 only on let's say PHP 8.4 and use Ubuntu's bundled libxml2 for the other builds?

.github/workflows/test.yml

jrfnl · 2025-01-28T17:20:50Z

Observation: by the looks of it, installing Homebrew and then installing libxml via Homebrew is only marginally faster and at times significantly slower than building from scratch.

Some Homebrew based builds:

https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/13012139556/job/36292353406 - 13 + 24 = 37 seconds
https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/12990605324/job/36226240335 - 19 + 73 = 92 seconds

versus some install from scratch based builds:

derrabus · 2025-01-28T19:22:27Z

Observation: by the looks of it, installing Homebrew and then installing libxml via Homebrew is only marginally faster and at times significantly slower than building from scratch.

According to https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md, Homebrew should already be present on the Ubuntu images from GitHub Actions. So we should not need to install Homebrew itself.

asispts · 2025-01-28T23:22:30Z

@derrabus Thanks. I've used that one.

@jrfnl @fredden I've also simplified the steps.

derrabus · 2025-01-29T09:50:04Z

Can we output the libxml version that PHP uses? I doubt that simply installing libxml will make PHP magically pick it up.

php -r 'echo LIBXML_DOTTED_VERSION . PHP_EOL;'

Worst case, we need to compile PHP against that libxml version ourselves. 😓

jrfnl · 2025-01-29T12:00:12Z

Can we output the libxml version that PHP uses? I doubt that simply installing libxml will make PHP magically pick it up.
php -r 'echo LIBXML_DOTTED_VERSION . PHP_EOL;'
Worst case, we need to compile PHP against that libxml version ourselves. 😓

@derrabus Interestingly enough, it appears it does (magically pick it up).

This can be seen via the test failure of the builds for the original push: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/12965930622/job/36168409300
Only after rebasing to include merged PR #797, did the test runs using libxml 2.12 pass.

I agree though that it wouldn't be a bad thing to have a debug step which outputs the version found.

.github/workflows/test.yml

jrfnl · 2025-02-06T00:00:37Z

Okay, for lack of answers to some of my questions about this PR and because I want to be sure that after all the updates to the PR, this still safeguards things properly, I've done some stress-testing.

These are the steps I took and my findings:

First off, using PHP 8.4 for this check is problematic as the tests are not being run on PHP 8.4 in the test job (as they are run in the code-coverage job instead), so I first changed the condition to install the updated version of libxml on PHP 8.3.
Expected: passing build
Result: passing build ✔️
With a passing build for that, I then reverted the changes from Tests: Fix libxml >= 2.12 compatibility #797 to verify that the build would fail correctly.
Expected: failing build
Result: passing build ❌ In other words: installing via homebrew does not get PHP to use the updated version of libxml. /cc @derrabus
Next, I changed the step to install libxml back to the script in commit bcc81fc which compiles libxml. The changes from Tests: Fix libxml >= 2.12 compatibility #797 are still reverted.
Expected: failing build
Result: failing build ✔️ Okay, so this way of updating libxml actually ensures that PHP uses this version.
Now, I reapplied the changes from Tests: Fix libxml >= 2.12 compatibility #797.
Expected: passing build
Result: passing build ✔️

I also tried to add a "DEBUG" step to get insight into the libxml version which is being used.
Unfortunately, using php -r 'echo "libxml version = ", LIBXML_DOTTED_VERSION, PHP_EOL;' does not work as it will always show the libxml version with which PHP was compiled, even when a newer libxml version is available on the system and is actually being used. /cc @fredden

While it is an extra step, installing xmllint and running xmllint --version will allow for displaying and verifying the real system version of libxml being used.

As the above means, we can't use homebrew to always get the latest libxml, we're back to using hard-coded versions, which is not great for maintenance.

My current thinking on this, is to explore using the GH API to retrieve the latest tag for the https://github.com/GNOME/libxml2 repo and see if we can get that working.

In the mean time, I think moving the hard-coded version from the update step to the matrix and having one PHP 8.* build run against libxml 2.12 and one against libxml 2.13 might be the best idea for now.

Having said that, the default version of libxml on ubuntu-latest appears to be 2.9.14, so maybe spreading the versions might even make sense. I.e. most builds use the default, one build uses a version in between (2.11) and one build uses the latest version 2.13.

Having the version in the matrix (or an update flag if we'd get the version via the GH API) will also allow for adding a note in the build name to draw attention to the different libxml version being used for select builds, making it more straight-forward to understand what's going on when a build fails.

jrfnl · 2025-02-06T00:05:32Z

@asispts Just checking - it looks like I can't push to your branch ? If you want to see the things I tested and the commits for some of the changes I envision for this PR, you can have a look here to see the code changes I made for the above tests: https://github.com/jrfnl/PHP_CodeSniffer/commits/asispts/bugfix/issue-767-action/

fredden · 2025-02-15T18:04:17Z

Unfortunately, using php -r 'echo "libxml version = ", LIBXML_DOTTED_VERSION, PHP_EOL;' does not work as it will always show the libxml version with which PHP was compiled, even when a newer libxml version is available on the system and is actually being used.

I've done some testing just now. From what I can tell, LIBXML_VERSION and LIBXML_DOTTED_VERSION both reflect the version of libxml which was used at compile time, whereas LIBXML_LOADED_VERSION shows the version that is currently in use. So this should work: php -r 'echo "LIBXML_LOADED_VERSION = ", LIBXML_LOADED_VERSION, PHP_EOL;'
It's not as pretty as the dotted variant, but it seems to show the correct information.

jrfnl · 2025-02-20T12:11:20Z

So this should work: php -r 'echo "LIBXML_LOADED_VERSION = ", LIBXML_LOADED_VERSION, PHP_EOL;' It's not as pretty as the dotted variant, but it seems to show the correct information.

Thanks @fredden, I can confirm that LIBXML_LOADED_VERSION works and would remove the need to install xmllint.

@asispts Just checking - do you still want to continue with this PR ? I'd like to get it merged in the same release as #797 as this would allow to safeguard cross-version support for various libxml versions via CI.

If you don't have the time or intention to finish this, please just let me know and I'll get it sorted out before the release myself.

jrfnl · 2025-02-27T16:58:35Z

For lack of response/progress on this PR, I've now opened PR #849 which brings the work done in this PR into a mergeable state. Feedback welcome.

github-actions bot added Type: bug Status: triage labels Jan 25, 2025

asispts mentioned this pull request Jan 25, 2025

Tests: make testBrokenRulesetMultiError() test compatible with LibXML 2.12+ #767

Closed

jrfnl mentioned this pull request Jan 25, 2025

Tests: Fix libxml >= 2.12 compatibility #797

Merged

11 tasks

asispts added 2 commits January 26, 2025 10:31

Run tests against libxml >= 2.12

3310914

Fix yaml format

bcc81fc

asispts force-pushed the bugfix/issue-767-action branch from ee93a6a to bcc81fc Compare January 26, 2025 03:31

jrfnl requested changes Jan 26, 2025

View reviewed changes

jrfnl added this to the 3.12.0 milestone Jan 26, 2025

jrfnl added Type: chores/QA and removed Type: bug Status: triage labels Jan 26, 2025

jrfnl added the Status: awaiting feedback label Jan 27, 2025

asispts added 6 commits January 27, 2025 20:36

Remove fail-fast

ceb01eb

Install libxml2 via homebrew

b54082e

Separate install libxml2 steps

e25ee62

Fix yaml

be3c27f

Update steps

19bf218

Use double quote

0ce96c6

asispts commented Jan 27, 2025

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

fredden reviewed Jan 28, 2025

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

Update comment and fix yaml

ad85bfa

asispts force-pushed the bugfix/issue-767-action branch from 7249a25 to ad85bfa Compare January 28, 2025 14:03

fredden reviewed Jan 28, 2025

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

Use pre-installed homebrew

1d715e3

fredden reviewed Jan 29, 2025

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

jrfnl mentioned this pull request Feb 27, 2025

GH Actions/tests: run tests against multiple libxml versions #849

Merged

jrfnl closed this in #849 Mar 7, 2025

github-actions bot removed the Status: awaiting feedback label Mar 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH Action: Add steps to test against libxml >= 2.12 #798

GH Action: Add steps to test against libxml >= 2.12 #798

asispts commented Jan 25, 2025

jrfnl commented Jan 25, 2025

asispts commented Jan 26, 2025

jrfnl left a comment

jrfnl Jan 26, 2025

jrfnl Jan 26, 2025

jrfnl Jan 26, 2025

asispts Jan 27, 2025 •

edited

Loading

jrfnl Jan 28, 2025 •

edited

Loading

derrabus commented Jan 26, 2025

jrfnl commented Jan 28, 2025

derrabus commented Jan 28, 2025

asispts commented Jan 28, 2025

derrabus commented Jan 29, 2025

jrfnl commented Jan 29, 2025 •

edited

Loading

jrfnl commented Feb 6, 2025

jrfnl commented Feb 6, 2025

fredden commented Feb 15, 2025

jrfnl commented Feb 20, 2025 •

edited

Loading

jrfnl commented Feb 27, 2025

GH Action: Add steps to test against libxml >= 2.12 #798

GH Action: Add steps to test against libxml >= 2.12 #798

Conversation

asispts commented Jan 25, 2025

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

jrfnl commented Jan 25, 2025

asispts commented Jan 26, 2025

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl Jan 26, 2025

Choose a reason for hiding this comment

jrfnl Jan 26, 2025

Choose a reason for hiding this comment

jrfnl Jan 26, 2025

Choose a reason for hiding this comment

asispts Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

jrfnl Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

derrabus commented Jan 26, 2025

jrfnl commented Jan 28, 2025

derrabus commented Jan 28, 2025

asispts commented Jan 28, 2025

derrabus commented Jan 29, 2025

jrfnl commented Jan 29, 2025 • edited Loading

jrfnl commented Feb 6, 2025

jrfnl commented Feb 6, 2025

fredden commented Feb 15, 2025

jrfnl commented Feb 20, 2025 • edited Loading

jrfnl commented Feb 27, 2025

asispts Jan 27, 2025 •

edited

Loading

jrfnl Jan 28, 2025 •

edited

Loading

jrfnl commented Jan 29, 2025 •

edited

Loading

jrfnl commented Feb 20, 2025 •

edited

Loading