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.

fredden · 2025-01-29T13:57:58Z

.github/workflows/test.yml

+        run: |
+          /home/linuxbrew/.linuxbrew/bin/brew install libxml2
+


Per recent suggestion, let's add the version of this library before & after the install the new version to (a) verify that the install worked as expected, and (b) help us know when the image starts to contain the version that we need.

Suggested change

run: |

/home/linuxbrew/.linuxbrew/bin/brew install libxml2

run: |

php -r 'echo "libxml version: ", LIBXML_DOTTED_VERSION, PHP_EOL;'

/home/linuxbrew/.linuxbrew/bin/brew install libxml2

php -r 'echo "libxml version: ", LIBXML_DOTTED_VERSION, PHP_EOL;'

That won't work because PHP is not installed yet at this point.

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

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

fredden Jan 29, 2025

derrabus Jan 29, 2025

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

Are you sure you want to change the base?

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

fredden Jan 29, 2025

Choose a reason for hiding this comment

derrabus Jan 29, 2025

Choose a reason for hiding this comment

asispts Jan 27, 2025 •

edited

Loading

jrfnl Jan 28, 2025 •

edited

Loading

jrfnl commented Jan 29, 2025 •

edited

Loading